Skip to content

feat(mcp,cli): report filter preflight and CLI label hex#284

Merged
adriannoes merged 5 commits into
feat/card-phase-erg/4-mcp-cli-create-card-phasefrom
feat/card-phase-erg/5-report-filter-and-cli-label
Jun 11, 2026
Merged

feat(mcp,cli): report filter preflight and CLI label hex#284
adriannoes merged 5 commits into
feat/card-phase-erg/4-mcp-cli-create-card-phasefrom
feat/card-phase-erg/5-report-filter-and-cli-label

Conversation

@adriannoes

@adriannoes adriannoes commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Analytics pipe seeding (306996636) required execute_graphql for phase inventory and phase-local creates. Spec: .cursor/dev-planning/specs/archive/card-phase-ergonomics/Stack after #277 (closed).

Depends on: #283


Objective

Close remaining boundary gaps from chaos-pipe:

  • Run validate_report_cards_filter on all MCP/CLI paths that accept filter (create/update/export pipe + org reports) — F6
  • CLI label create/update uses normalize_label_color (F7)

Improvements

  • Top-level current_phase mistake caught before GraphQL with a clear message.
  • #F00 works like the live API; red rejected locally.

@gbrlcustodio gbrlcustodio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One correctness risk to verify against the API, one altitude note, one cleanup, one docs gap. The filter-narrowing items are plausible (the validator is stricter than the previously-passthrough path), not confirmed regressions: the repo has no ReportCardsFilter schema to check against.

Comment thread packages/mcp/src/pipefy_mcp/tools/report_tools.py Outdated
Comment thread packages/mcp/src/pipefy_mcp/tools/report_tools.py Outdated
Comment thread packages/cli/src/pipefy_cli/commands/label.py Outdated
Comment thread packages/mcp/src/pipefy_mcp/tools/report_tools.py
@adriannoes adriannoes force-pushed the feat/card-phase-erg/4-mcp-cli-create-card-phase branch from db4b77e to 2e0d0b8 Compare June 10, 2026 15:30
@adriannoes adriannoes force-pushed the feat/card-phase-erg/5-report-filter-and-cli-label branch from 01c4788 to 293a18f Compare June 10, 2026 15:30
@adriannoes

Copy link
Copy Markdown
Collaborator Author

Thanks @gbrlcustodio — addressed in 293a18f: integer filter.queries[].value scalars coerce to strings before preflight; shared label name/color CLI validators; consistent ReportCardsFilter shape docstrings on all six filter paths. SDK-boundary enforcement deferred to a follow-up (consistent with boundary-only validation in #289).

@gbrlcustodio

Copy link
Copy Markdown
Collaborator

Consolidating the four filter and label threads

The four open threads on this PR are one structural issue, not four. Filter and color validation is a decision (is this input valid, and what is its normalized form?) wired at twelve effect sites: the six MCP report tools and the six CLI commands. Every one of those sites forwards through a single SDK layer: report_service (create_pipe_report, update_pipe_report, create_organization_report, update_organization_report, export_pipe_report, export_organization_report) and pipe_config_service (create_label, update_label). The validation sits one layer too high.

The move

Push normalize and validate down to those SDK methods, and let each surface only shape the error.

# report_service.py: one decision point that all six methods call
def _prepare_filter(filter_obj):
    if filter_obj is None:
        return None
    normalized = normalize_report_cards_filter(filter_obj)
    msg = validate_report_cards_filter(normalized)
    if msg is not None:
        raise ValueError(msg)  # or a small typed subclass for precise catches
    return normalized
# each method: input_obj["filter"] = _prepare_filter(filter)

# pipe_config_service.py: normalize at the funnel
async def create_label(self, pipe_id, name, color):
    color = normalize_label_color(color)  # already raises ValueError on bad input
    ...
async def update_label(self, label_id, **attrs):
    if "color" in attrs:
        attrs["color"] = normalize_label_color(attrs["color"])

Then delete the twelve leaf guards. MCP catches the error and returns build_report_error_payload; the CLI catches it and exits non-zero. The color path already raises ValueError, which both surfaces already map.

What this resolves

  • All-paths invariant (the wiring thread): holds by construction. A seventh report path, a new tool, or a direct SDK consumer is covered without remembering to call a helper.
  • Label duplication and the redundant empty-color guard: gone. No per-surface color helper, no copy-paste between create and update.
  • The current_phase docstring guidance: stops being load-bearing. The central validator returns one clear, shape-pointing message, so a per-tool docstring line becomes optional rather than something repeated on six tools (and re-broken, see below).
  • The numeric-versus-API strictness question: not answered by this move, but localized. It collapses from a property of twelve sites to one seam next to the GraphQL call, which is where to verify it and relax it.

Trade-offs

  • The SDK methods would raise on invalid input instead of forwarding. That is the more correct boundary and keeps the decision inside the owned abstraction, but it is a contract change and the SDK tests move with it.
  • CLI error timing shifts from parse-time (typer.BadParameter in a parameter validator) to call-time inside the command body. Same non-zero exit and message; a thin catch can preserve the BadParameter feel if that matters.
  • Larger diff than the current fix commit, but it nets less code: one decision point replacing twelve, with the leaf guards and duplicated normalization deleted.

Needs fixing regardless of the above

The docstring change in 293a18f introduced a regression. The four newly-documented tools use f-string docstrings (f"""..."""). Python does not treat an f-string as a docstring, so __doc__ is None for create_organization_report, update_organization_report, export_pipe_report, and export_organization_report, and their MCP descriptions are now empty (confirmed with ast.get_docstring(...) == None). A variable cannot go into a docstring at all; even "text" + VAR yields None. Inline the guidance as a plain string literal the way create_pipe_report and update_pipe_report already do. Tracked on that thread.

@adriannoes adriannoes force-pushed the feat/card-phase-erg/5-report-filter-and-cli-label branch from 293a18f to d0260f5 Compare June 10, 2026 17:54
@adriannoes adriannoes force-pushed the feat/card-phase-erg/4-mcp-cli-create-card-phase branch from 2e0d0b8 to 66d0703 Compare June 10, 2026 17:54
@adriannoes

Copy link
Copy Markdown
Collaborator Author

Thanks @gbrlcustodio — you convinced us. The consolidated move is implemented in #291 (PR 7 of this stack, stacked on #285): ReportService now normalizes the filter and raises ValueError via a single _prepared_report_cards_filter decision point wired into all six methods, PipeConfigService.create_label/update_label normalize the hex color, and the twelve leaf guards are deleted. MCP maps ValueError to the same build_report_error_payload envelope; the CLI maps it to exit code 2 (typer.BadParameter on report commands), so messages and exit codes are unchanged. The #280 naming finding rides along there (validate_report_cards_filter -> report_cards_filter_error).

The f-string docstring regression was fixed separately on this PR in b7dbc4e (plain literals + regression tests asserting non-empty descriptions and the current_phase guidance on all six filter paths) — per your note, with the central validator in place that guidance is now informative rather than load-bearing.

Comment thread packages/cli/src/pipefy_cli/commands/_common.py
@adriannoes adriannoes force-pushed the feat/card-phase-erg/4-mcp-cli-create-card-phase branch from 598f553 to ca568bb Compare June 11, 2026 20:01
adriannoes and others added 5 commits June 11, 2026 17:04
… CLI

Fail fast on malformed ReportCardsFilter (including export paths); CLI
label create/update uses normalize_label_color (#RGB expanded to #RRGGBB).
…dation

Normalize integer query values to strings before ReportCardsFilter
preflight; apply normalized filters on MCP paths. Share label name/color
CLI validation; align filter-shape docstrings across all six report tools.
Python does not treat f-strings as docstrings, so the four report tools
documented via f"""...{_REPORT_FILTER_SHAPE_DOC}""" exposed empty MCP
descriptions. Inline the ReportCardsFilter guidance as plain literals and
add regression tests asserting non-empty descriptions and the
current_phase warning on all six filter paths.

Co-authored-by: Gabriel Custodio <gbrlcustodio@users.noreply.github.com>
Align validate_label_name_cli and validate_label_color_cli with
validate_report_filter_cli so validation errors share the same format.
@adriannoes adriannoes force-pushed the feat/card-phase-erg/5-report-filter-and-cli-label branch from d0260f5 to 26c86b3 Compare June 11, 2026 20:06
@adriannoes

Copy link
Copy Markdown
Collaborator Author

Rebased onto latest feat/card-phase-erg/4-mcp-cli-create-card-phase (skipped duplicate create_card commit) and pushed 26c86b3 addressing @Danielmoraisg's nit: label name/color validators now use typer.BadParameter like validate_report_filter_cli.

Tests run locally:

  • uv run pytest packages/cli/tests/test_label_webhook_commands.py packages/cli/tests/test_report_pipe_filter_commands.py packages/mcp/tests/tools/test_report_tools.py packages/sdk/tests/test_report_filter_preflight.py packages/sdk/tests/test_label_color.py -m "not integration"94 passed
  • Full suite: 3065 passed; 6 failures are pre-existing stack drift (parity/docs rows for phase tools + CLI help golden from stacked PRs 6+), unrelated to this PR's scope.

Ready for manual merge when CI is green.

@adriannoes adriannoes merged commit ccb361f into feat/card-phase-erg/4-mcp-cli-create-card-phase Jun 11, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants