Improve CLI search and fetch signaling#11
Conversation
PR Review: fix/search-fetch-signaling-10PR: #11 Improve CLI search and fetch signaling SummaryThis PR simplifies the CLI search query shape, rejects several ambiguous DSL forms before calling the SDK, updates README/help examples, and fixes Change inventory
Apparent intent from commits:
FindingsBugsB1 - Lowercase
|
PR Review: fix/search-fetch-signaling-10PR: #11 Improve CLI search and fetch signaling SummaryThis PR simplifies CLI search syntax, rejects several ambiguous query forms before calling the SDK, updates README/help examples, and fixes unresolved Change inventory
Apparent intent from commits:
FindingsBugsB1 - Documented multi-word
|
| " ctxd search text:deployment\n" | ||
| " ctxd search application:slack text:deployment\n" | ||
| "\n" | ||
| "Query format:\n" | ||
| " application:<app> text:<term>\n" | ||
| " application:<app> is optional; omit it to search all connected apps.\n" | ||
| " If application:<app> is present, it must be the first token.\n" | ||
| " Only one application filter is supported.\n" | ||
| " Use one text term; multi-word, quoted, and parenthesized text values are not supported." |
PR Review: fix/search-fetch-signaling-10PR: #11 Improve CLI search and fetch signaling SummaryThis PR narrows CLI search syntax to a predictable leading Change inventory
Apparent intent from commits:
FindingsBugsB1 - Repeated
|
PR Review: fix/search-fetch-signaling-10PR: #11 Improve CLI search and fetch signaling SummaryThis PR narrows CLI search syntax to a leading Change inventory
Apparent intent from commits:
FindingsBugsB1 - Application-scoped bare terms still bypass the documented query shape
Design gapsNone found beyond B1. Side effects & risksNone found. Edge cases & race conditionsNone found. Dead codeNone found. The changed private helpers are reachable through
Test coverage gapsT1 - No test rejects app-scoped searches without
|
PR Review: fix/search-fetch-signaling-10PR: #11 Improve CLI search and fetch signaling SummaryThis PR narrows CLI search syntax to a leading Change inventory
Apparent intent from commits:
FindingsBugsNone found. Design gapsNone found. Side effects & risksNone found. Edge cases & race conditionsNone found. Dead codeNone found. The changed private helpers are reachable through
Test coverage gapsNone found. The changed CLI behavior has focused coverage for empty app filters, grouped app filters, trailing app filters, repeated app filters, uppercase boolean clauses, lowercase Test regressionsNone found. No test files were deleted, no skips/xfails were added, and the assertion changes match the intentional simplified CLI contract. CI failuresNone found.
Unresolved PR commentsNone found. The previous top-level review comments are addressed in the current head: lowercase Positive observations
Recommendations
Final verdictVerdict: Approve Approving: no critical/high/medium findings remain, CI is green for the current head, and prior review findings have been addressed with focused tests. |
Summary
application:<app>followed by one backend-supportedtext:<term>; omitting the application filter searches all connected apps.AND/ORclauses, quoted text values, parenthesized text values, and multi-word text values.Verification
uv run pytest tests/test_cli.pyCTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytestgit diff --checkFixes #10