Skip to content

Improve CLI search and fetch signaling#11

Open
juanpabloguerra16 wants to merge 7 commits into
mainfrom
fix/search-fetch-signaling-10
Open

Improve CLI search and fetch signaling#11
juanpabloguerra16 wants to merge 7 commits into
mainfrom
fix/search-fetch-signaling-10

Conversation

@juanpabloguerra16

@juanpabloguerra16 juanpabloguerra16 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Simplify CLI search guidance around one predictable query shape: optional leading application:<app> followed by one backend-supported text:<term>; omitting the application filter searches all connected apps.
  • Update CLI validation to reject ambiguous or unsupported query forms: empty app filters, app filters after text, repeated app filters, grouped app filters, AND/OR clauses, quoted text values, parenthesized text values, and multi-word text values.
  • Preserve the fetch signaling fix so unresolved document payloads with errors return a non-zero exit code.
  • Update README and CLI help examples to match the simplified search format.

Verification

  • uv run pytest tests/test_cli.py
  • CTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytest
  • git diff --check

Fixes #10

@juanpabloguerra16

Copy link
Copy Markdown
Contributor Author

PR Review: fix/search-fetch-signaling-10

PR: #11 Improve CLI search and fetch signaling
Base: main at 7b5cb54
Head: 090dfedf2b41eed3a655d7d1bd94040c048ccff7
Commits: 3
Files changed: 3

Summary

This PR simplifies the CLI search query shape, rejects several ambiguous DSL forms before calling the SDK, updates README/help examples, and fixes fetch so unresolved document payloads produce a non-zero CLI exit code. The implementation is small and well covered, but the boolean-operator validation is broader than the stated simplified query contract: it rejects lowercase and/or when users include those words as ordinary search terms.

Change inventory

File Change type Subsystem Risk
README.md modified Docs low
src/ctxd/cli.py modified CLI medium
tests/test_cli.py modified Tests low

Apparent intent from commits:

  • 1a1c54f improves CLI search/fetch signaling.
  • f641625 simplifies CLI search query guidance.
  • 090dfed rejects complex text search forms.

Findings

Bugs

B1 - Lowercase and / or are rejected as DSL operators

  • Location: src/ctxd/cli.py:240

  • Severity: medium

  • Description: _validate_boolean_operators() uppercases every token before checking for AND / OR, so ordinary lowercase words in a simple search are rejected. For example, ctxd search text:research and development exits 1 with "AND/OR clauses are not supported" even though the help text says to use simple text terms and the PR intent is to reject ambiguous DSL boolean clauses.

  • Evidence: The validator uses token.upper() in operators at src/ctxd/cli.py:243. I reproduced the failure locally:

    $ uv run python -c "from ctxd.cli import main; import sys; sys.exit(main(['search','text:research','and','development']))"
    Invalid search query: AND/OR clauses are not supported. Use application:<app> text:<terms>, or omit application:<app> to search all apps.
    
  • Suggested fix:

    # src/ctxd/cli.py:240
    def _validate_boolean_operators(tokens: Sequence[str]) -> None:
        operators = {"AND", "OR"}
        for token in tokens:
            if token in operators:
                raise ValueError(
                    "Invalid search query: AND/OR clauses are not supported. "
                    "Use application:<app> text:<terms>, or omit application:<app> to search all apps."
                )

    Add a regression test alongside test_cli_search_rejects_boolean_operators():

    def test_cli_search_allows_lowercase_and_or_as_text_terms() -> None:
        stdout = StringIO()
    
        with patch(
            "ctxd.cli.Client.search",
            return_value=type(
                "SearchResultLike",
                (),
                {
                    "model_dump": lambda self: {
                        "results": [],
                        "error": None,
                        "dsl_parse_error": None,
                    }
                },
            )(),
        ) as search, redirect_stdout(stdout):
            exit_code = main(["search", "text:research", "and", "development"])
    
        assert exit_code == 0
        search.assert_called_once_with("text:research and development")
        assert '"results": []' in stdout.getvalue()

Design gaps

None found. The previous text:deployment application:slack CLI shape is now rejected, but that is consistent with the PR description and updated docs, which intentionally require any application:<app> filter to lead the query.

Side effects & risks

None found.

Edge cases & race conditions

None found beyond B1.

Dead code

None found. The new private helpers are reachable from main() through the search path, and _document_payload_is_unresolved() is reachable through _payload_has_error() for JSON output.

Test coverage gaps

T1 - No coverage for lowercase boolean words as normal text

  • Changed code: src/ctxd/cli.py:240
  • Gap type: missing edge case
  • Severity: medium
  • Description: Tests cover uppercase AND as an unsupported DSL operator at tests/test_cli.py:449, but they do not cover lowercase and / or as ordinary words in simple text terms. That gap allowed B1.
  • Suggestion: Add the regression test shown in B1.

Test regressions

None found. No test files were deleted, no skips/xfails were added, and the changed expectations align with the intentional simplified search format. The fetch error signaling is covered for both human-readable and JSON output at tests/test_cli.py:347 and tests/test_cli.py:360.

CI failures

None found.

  • CI status: Test passed for head 090dfedf2b41eed3a655d7d1bd94040c048ccff7 in GitHub Actions run 26912989925, job 79395629101.

  • Skipped jobs: Publish to PyPI was skipped, not failed, in job 79395669061.

  • Local verification:

    uv run pytest tests/test_cli.py
    # 34 passed in 0.17s
    
    CTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytest
    # 50 passed in 0.26s
    
    git diff --check 7b5cb54fe2768f70fe8dd745688e4f3199d825f3...fix/search-fetch-signaling-10
    # no output
    

Unresolved PR comments

None found. Raw GitHub API checks for inline review comments, reviews, and top-level issue comments all returned empty arrays; no stale approvals or change-request reviews exist.

Positive observations

  • The new search validation fails before calling Client.search(), and tests assert search.assert_not_called() for invalid query forms.
  • The unresolved-document fetch path now returns exit code 1 in both text and JSON output modes.
  • README and CLI help examples are consistent with the new leading-application query shape.
  • The PR includes focused tests for empty app filters, grouped app filters, trailing app filters, repeated app filters, quoted text, parenthesized text, and non-zero fetch errors.

Recommendations

  1. Fix B1 and add T1 before merging. The current validator blocks common plain-English searches that include and or or.
  2. Keep the intentional rejection of trailing application:<app> documented, since that is a user-visible CLI contract change.
  3. Separately from this PR, update the GitHub Actions versions flagged by the successful CI run's Node.js 20 deprecation warning before GitHub's 2026 cutoff dates.

Final verdict

Verdict: Request follow-up

B1 is a medium user-facing CLI parsing bug introduced on the search hot path, with T1 documenting the missing regression coverage. CI is green and there are no unresolved PR comments, but B1 should be fixed before merge.

@juanpabloguerra16

Copy link
Copy Markdown
Contributor Author

PR Review: fix/search-fetch-signaling-10

PR: #11 Improve CLI search and fetch signaling
Base: main at 7b5cb54
Head: e1b50ece0b8971f612f9b4c3218032c8bcfb1184
Commits: 4
Files changed: 3

Summary

This PR simplifies CLI search syntax, rejects several ambiguous query forms before calling the SDK, updates README/help examples, and fixes unresolved fetch results so they exit non-zero. The previous review finding about lowercase and / or has been fixed and covered by a regression test. One blocking issue remains: the CLI now documents and forwards multi-word text:<terms> queries even though the linked issue documents that this exact unquoted DSL shape parse-errors at the backend, and the current validator still accepts shell-stripped quoted values in real CLI usage.

Change inventory

File Change type Subsystem Risk
README.md modified Docs medium
src/ctxd/cli.py modified CLI medium
tests/test_cli.py modified Tests medium

Apparent intent from commits:

  • 1a1c54f improves CLI search/fetch signaling.
  • f641625 simplifies CLI search query guidance.
  • 090dfed rejects complex text search forms.
  • e1b50ec allows lowercase boolean words in search text, addressing the prior review comment.

Findings

Bugs

B1 - Documented multi-word text: queries are forwarded in the backend-invalid shape

  • Location: src/ctxd/cli.py:168, src/ctxd/cli.py:227, src/ctxd/cli.py:240, README.md:42

  • Severity: medium

  • Description: The PR now documents ctxd search application:google_drive text:incident response and tests main(["search", "application:slack", "text:deployment process"]) as a successful path, but the CLI sends the query through unchanged as application:slack text:deployment process. Issue search/fetch: silent & misleading error/semantics signaling (DSL forms, multi-app filters, result cap, exit codes) #10, which this PR fixes, documents that unquoted multi-word text: values like text:incident postmortem return a DSL parse error. There is no backend/client contract change in this repo that makes the new shape valid; AsyncClient.search() still forwards the query string verbatim at src/ctxd/async_client.py:40.

  • Evidence:

    • Issue search/fetch: silent & misleading error/semantics signaling (DSL forms, multi-app filters, result cap, exit codes) #10 says text:incident postmortem errors and asks to fix the misleading suggestion, not to start forwarding that same invalid shape as the documented happy path.

    • The old CLI normalized a single shell-stripped multi-word text: token into a quoted value at base src/ctxd/cli.py:157-180; this PR removes that normalization and now just joins argv tokens at src/ctxd/cli.py:168.

    • The new test asserts the backend call is exactly the unquoted multi-word shape:

      # tests/test_cli.py:630
      exit_code = main(["search", "application:slack", "text:deployment process"])
      assert exit_code == 0
      search.assert_called_once_with("application:slack text:deployment process")
    • In actual shell usage, text:"incident response" is received as a single argv token containing text:incident response; the current validator accepts that same shape, even though the help text says quoted text values are unsupported.

  • Suggested fix: Until the backend accepts the simplified multi-word text:<terms> contract, reject shell-stripped multi-word text: values locally and avoid documenting bare continuation terms as a supported DSL value. For example:

    # src/ctxd/cli.py
    def _validate_text_filters(tokens: Sequence[str]) -> None:
        for index, token in enumerate(tokens):
            if not token.lower().startswith("text:"):
                continue
    
            value = token[len("text:") :]
            if value.startswith(("\"", "'", "(")) or value.endswith(("\"", "'", ")")):
                raise ValueError(
                    "Invalid search query: quoted and parenthesized text values are not supported. "
                    "Use a single text term, for example text:incident."
                )
            if re.search(r"\s", value):
                raise ValueError(
                    "Invalid search query: multi-word text values are not supported by the current DSL. "
                    "Use a single text term, for example text:incident."
                )
            if index + 1 < len(tokens) and not tokens[index + 1].lower().startswith(
                ("application:", "text:")
            ):
                raise ValueError(
                    "Invalid search query: multi-word text values are not supported by the current DSL. "
                    "Use a single text term, for example text:incident."
                )

    Then update the help/README examples from text:incident response to a shape the backend accepts, or add a backend-backed integration/contract test proving that application:<app> text:<terms> is now valid before keeping the new examples.

Design gaps

None beyond B1.

Side effects & risks

None found.

Edge cases & race conditions

None found.

Dead code

None found. The changed private helpers are reachable through main():

  • _normalize_search_query() and _validate_search_query() are called by the search command at src/ctxd/cli.py:35-36.
  • _split_search_query(), _validate_raw_text_filters(), _validate_application_filters(), _validate_text_filters(), and _validate_boolean_operators() are called from _validate_search_query() at src/ctxd/cli.py:172-177.
  • _payload_has_error() is called from JSON output at src/ctxd/cli.py:341.
  • _document_payload_is_unresolved() is called from _payload_has_error() at src/ctxd/cli.py:387.

Test coverage gaps

T1 - No test proves the documented multi-word text: query works against the real search contract

  • Changed code: src/ctxd/cli.py:168, tests/test_cli.py:613

  • Gap type: missing test / weak assertion

  • Severity: medium

  • Description: test_cli_search_keeps_multi_word_text_simple() only asserts that the mocked SDK receives application:slack text:deployment process; it does not prove that the search backend accepts this DSL. Given issue search/fetch: silent & misleading error/semantics signaling (DSL forms, multi-app filters, result cap, exit codes) #10's documented parse error for unquoted multi-word text: values, the test locks in the risky forwarding behavior rather than validating successful search semantics.

  • Suggestion: Either add a backend contract test for the simplified query form, or change the unit test to assert local rejection until that contract exists:

    def test_cli_search_rejects_shell_stripped_multi_word_text_filter() -> None:
        stderr = StringIO()
    
        with patch("ctxd.cli.Client.search") as search, patch("sys.stderr", stderr):
            exit_code = main(["search", "application:slack", "text:deployment process"])
    
        assert exit_code == 1
        search.assert_not_called()
        assert "multi-word text values are not supported" in stderr.getvalue()

Test regressions

None found. No test files were deleted, no skips/xfails were added, and no existing assertion was weakened without an implementation change. The prior review's lowercase boolean-word gap is now covered at tests/test_cli.py:460.

CI failures

None found.

  • CI status: Test passed for head e1b50ece0b8971f612f9b4c3218032c8bcfb1184 in GitHub Actions run 26915104750, job 79402792351.

  • Skipped jobs: Publish to PyPI was skipped, not failed, in job 79402838915.

  • Local verification:

    uv run pytest tests/test_cli.py
    # 35 passed in 0.17s
    
    CTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytest
    # 51 passed in 0.27s
    
    git diff --check 7b5cb54fe2768f70fe8dd745688e4f3199d825f3...fix/search-fetch-signaling-10
    # no output
    

Unresolved PR comments

None found.

The prior top-level review comment #issuecomment-4616970482 requested fixing lowercase and / or rejection. That ask is addressed in the current head by src/ctxd/cli.py:243 checking exact AND / OR tokens only and by the regression test at tests/test_cli.py:460. Raw GitHub API checks for inline review comments and reviews returned empty arrays; there are no stale approvals or change-request reviews.

Positive observations

  • The previous review's B1/T1 is fixed and covered by test_cli_search_allows_lowercase_and_or_as_text_terms().
  • Invalid app filters, repeated app filters, grouped app filters, uppercase boolean clauses, quoted text, and parenthesized text are rejected before calling Client.search().
  • Unresolved document fetches now return exit code 1 in both text and JSON modes.
  • CI and local tests are green on the updated head.

Recommendations

  1. Fix B1/T1 before merging by aligning the documented CLI query form with the backend search contract, or by adding a real contract test proving the backend now accepts application:<app> text:<terms>.
  2. Keep the lowercase boolean-word regression test from e1b50ec; it covers the prior review finding.
  3. Separately from this PR, update the GitHub Actions versions flagged by the successful CI run's Node.js 20 deprecation warning before GitHub's 2026 cutoff dates.

Final verdict

Verdict: Request follow-up

B1 and T1 should be addressed before merge because the CLI currently advertises and locks in a multi-word text: query shape that issue #10 identifies as backend-invalid, with only mocked unit coverage proving the SDK call string.

Comment thread src/ctxd/cli.py
Comment on lines +122 to +130
" 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."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@fostiropoulos lmk what you think

@juanpabloguerra16

Copy link
Copy Markdown
Contributor Author

PR Review: fix/search-fetch-signaling-10

PR: #11 Improve CLI search and fetch signaling
Base: main at 7b5cb54
Head: d03033d04f47af960618e54f469713a1b35485f2
Commits: 5
Files changed: 3

Summary

This PR narrows CLI search syntax to a predictable leading application:<app> plus one text:<term> shape, rejects several ambiguous forms locally, updates README/help examples, and fixes unresolved fetch results so they exit non-zero. The prior findings for lowercase and / or and shell-stripped multi-word text: values are addressed. One validation gap remains: repeated text: filters still bypass the new one-term contract.

Change inventory

File Change type Subsystem Risk
README.md modified Docs low
src/ctxd/cli.py modified CLI medium
tests/test_cli.py modified Tests medium

Apparent intent from commits:

  • 1a1c54f improves CLI search/fetch signaling.
  • f641625 simplifies CLI search query guidance.
  • 090dfed rejects complex text search forms.
  • e1b50ec allows lowercase boolean words in search text.
  • d03033d rejects unsupported multi-word text filters.

Findings

Bugs

B1 - Repeated text: filters bypass the one-term search contract

  • Location: src/ctxd/cli.py:226

  • Severity: medium

  • Description: The help text and PR body now describe one supported text:<term> filter, but _validate_text_filters() validates each text: token independently and never rejects a second one. As a result, ctxd search application:slack text:incident text:response is accepted and forwarded to the backend, even though it is outside the documented simplified query shape and can reintroduce ambiguous backend DSL behavior.

  • Evidence: I reproduced this locally with the SDK call mocked:

    ['search', 'application:slack', 'text:incident', 'text:response']
    code=0
    call=call('application:slack text:incident text:response')
    

    The validator rejects bare continuation terms at src/ctxd/cli.py:242-248, but because a repeated token starts with text:, it is treated as valid rather than as a second text term.

  • Suggested fix:

    # src/ctxd/cli.py:226
    def _validate_text_filters(tokens: Sequence[str]) -> None:
        text_positions: list[int] = []
        for index, token in enumerate(tokens):
            if not token.lower().startswith("text:"):
                continue
    
            text_positions.append(index)
            value = token[len("text:") :]
            if value.startswith(("\"", "'", "(")) or value.endswith(("\"", "'", ")")):
                raise ValueError(
                    "Invalid search query: quoted and parenthesized text values are not supported. "
                    "Use a single text term, for example text:incident."
                )
            if re.search(r"\s", value):
                raise ValueError(
                    "Invalid search query: multi-word text values are not supported by the current DSL. "
                    "Use a single text term, for example text:incident."
                )
            if index + 1 < len(tokens) and not tokens[index + 1].lower().startswith(
                ("application:", "text:")
            ):
                raise ValueError(
                    "Invalid search query: multi-word text values are not supported by the current DSL. "
                    "Use a single text term, for example text:incident."
                )
    
        if len(text_positions) > 1:
            raise ValueError(
                "Invalid search query: only one text filter is supported. "
                "Use a single text term, for example text:incident."
            )

Design gaps

None found beyond B1.

Side effects & risks

None found.

Edge cases & race conditions

None found.

Dead code

None found. The changed private helpers are reachable through main():

  • _normalize_search_query() and _validate_search_query() are called at src/ctxd/cli.py:35-36.
  • _split_search_query(), _validate_raw_text_filters(), _validate_application_filters(), _validate_boolean_operators(), and _validate_text_filters() are called from _validate_search_query() at src/ctxd/cli.py:171-176.
  • _payload_has_error() is called from JSON output at src/ctxd/cli.py:352.
  • _document_payload_is_unresolved() is called from _payload_has_error() at src/ctxd/cli.py:398.

Test coverage gaps

T1 - No test rejects multiple text: filters

  • Changed code: src/ctxd/cli.py:226

  • Gap type: missing negative test

  • Severity: medium

  • Description: Tests now cover shell-stripped multi-word text and bare continuation terms at tests/test_cli.py:612 and tests/test_cli.py:623, but they do not cover repeated text: filters. That leaves the one-term search contract unenforced.

  • Suggestion:

    def test_cli_search_rejects_repeated_text_filters() -> None:
        stderr = StringIO()
    
        with patch("ctxd.cli.Client.search") as search, patch("sys.stderr", stderr):
            exit_code = main(
                ["search", "application:slack", "text:incident", "text:response"]
            )
    
        assert exit_code == 1
        search.assert_not_called()
        assert "only one text filter is supported" in stderr.getvalue()

Test regressions

None found. No test files were deleted, no skips/xfails were added, and the assertion changes match the intentional simplified CLI contract.

CI failures

None found.

  • CI status: Test passed for head d03033d04f47af960618e54f469713a1b35485f2 in GitHub Actions run 26915614578, job 79404422821.

  • Skipped jobs: Publish to PyPI was skipped, not failed, in job 79404456805.

  • Local verification:

    uv run pytest tests/test_cli.py
    # 37 passed in 0.15s
    
    CTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytest
    # 53 passed in 0.25s
    
    git diff --check 7b5cb54fe2768f70fe8dd745688e4f3199d825f3...fix/search-fetch-signaling-10
    # no output
    

Unresolved PR comments

None found.

The prior review comments are addressed in the current head: lowercase and / or is covered at tests/test_cli.py:460, and multi-word text rejection is covered at tests/test_cli.py:612 and tests/test_cli.py:623. The new inline comment at discussion_r3359700921 is a question ("lmk what you think") on the help text, not a requested code change.

Positive observations

  • The current help text now matches the backend-supported single-term shape better than the previous revision.
  • The CLI rejects invalid app filters, repeated app filters, grouped app filters, uppercase boolean clauses, quoted text, parenthesized text, shell-stripped multi-word text, and bare continuation terms before calling Client.search().
  • Unresolved document fetches now return exit code 1 in both text and JSON modes.
  • CI and local tests are green on the updated head.

Recommendations

  1. Fix B1 and add T1 before merging so the validator fully enforces the documented one-term CLI contract.
  2. Keep the existing regression tests for lowercase boolean words and multi-word text rejection.
  3. Separately from this PR, update the GitHub Actions versions flagged by the successful CI run's Node.js 20 deprecation warning before GitHub's 2026 cutoff dates.

Final verdict

Verdict: Request follow-up

B1/T1 is a medium validation gap on the exact search path this PR is trying to simplify. CI is green and the earlier findings are fixed, but repeated text: filters should be rejected before merge.

@juanpabloguerra16

Copy link
Copy Markdown
Contributor Author

PR Review: fix/search-fetch-signaling-10

PR: #11 Improve CLI search and fetch signaling
Base: main at 7b5cb54
Head: 0424c0d715d612d5cece99c54eaf5500096b61da
Commits: 6
Files changed: 3

Summary

This PR narrows CLI search syntax to a leading application:<app> plus one text:<term> shape, rejects ambiguous DSL forms locally, updates README/help examples, and fixes unresolved fetch results so they exit non-zero. The prior findings for lowercase boolean words, multi-word text: values, and repeated text: filters are addressed. One validation gap remains around application-scoped searches: app-only and app-plus-bare-term queries still bypass the documented shape.

Change inventory

File Change type Subsystem Risk
README.md modified Docs low
src/ctxd/cli.py modified CLI medium
tests/test_cli.py modified Tests medium

Apparent intent from commits:

  • 1a1c54f improves CLI search/fetch signaling.
  • f641625 simplifies CLI search query guidance.
  • 090dfed rejects complex text search forms.
  • e1b50ec allows lowercase boolean words in search text.
  • d03033d rejects unsupported multi-word text filters.
  • 0424c0d rejects repeated text search filters.

Findings

Bugs

B1 - Application-scoped bare terms still bypass the documented query shape

  • Location: src/ctxd/cli.py:194, src/ctxd/cli.py:226

  • Severity: medium

  • Description: The CLI help and PR body say the supported shape is application:<app> text:<term> with a single text term, but validation still accepts ctxd search application:slack deployment and ctxd search application:slack. Both are outside the documented shape. The first also sends a bare word after an app filter, which is the kind of ambiguous/orphan text form this PR is trying to eliminate.

  • Evidence: Reproduced locally with Client.search mocked:

    ['search', 'application:slack'] code=0 call=call('application:slack')
    ['search', 'application:slack', 'deployment'] code=0 call=call('application:slack deployment')
    

    _validate_application_filters() only checks app position/count/value, and _validate_text_filters() only validates tokens that start with text:. With no text: token present, the app-scoped query is forwarded unchanged.

  • Suggested fix:

    # src/ctxd/cli.py
    def _validate_search_query(query: str) -> None:
        _validate_raw_text_filters(query)
        tokens = _split_search_query(query)
        _validate_application_filters(tokens)
        _validate_boolean_operators(tokens)
        _validate_text_filters(tokens)
        _validate_application_scoped_search_shape(tokens)
    
    
    def _validate_application_scoped_search_shape(tokens: Sequence[str]) -> None:
        if not tokens or not tokens[0].lower().startswith("application:"):
            return
        if len(tokens) != 2 or not tokens[1].lower().startswith("text:"):
            raise ValueError(
                "Invalid search query: application:<app> must be followed by one text:<term> filter, "
                "for example application:slack text:deployment."
            )

    Add a regression test:

    @pytest.mark.parametrize(
        "query",
        [
            ["search", "application:slack"],
            ["search", "application:slack", "deployment"],
        ],
    )
    def test_cli_search_rejects_application_filter_without_text_filter(
        query: list[str],
    ) -> None:
        stderr = StringIO()
    
        with patch("ctxd.cli.Client.search") as search, patch("sys.stderr", stderr):
            exit_code = main(query)
    
        assert exit_code == 1
        search.assert_not_called()
        assert "must be followed by one text:<term>" in stderr.getvalue()

Design gaps

None found beyond B1.

Side effects & risks

None found.

Edge cases & race conditions

None found.

Dead code

None found. The changed private helpers are reachable through main():

  • _normalize_search_query() and _validate_search_query() are called at src/ctxd/cli.py:35-36.
  • _split_search_query(), _validate_raw_text_filters(), _validate_application_filters(), _validate_boolean_operators(), and _validate_text_filters() are called from _validate_search_query() at src/ctxd/cli.py:171-176.
  • _payload_has_error() is called from JSON output at src/ctxd/cli.py:360.
  • _document_payload_is_unresolved() is called from _payload_has_error() at src/ctxd/cli.py:406.

Test coverage gaps

T1 - No test rejects app-scoped searches without text:<term>

  • Changed code: src/ctxd/cli.py:194, src/ctxd/cli.py:226
  • Gap type: missing negative test
  • Severity: medium
  • Description: Tests cover app filter position/count, multi-word text rejection, repeated text rejection, and valid application:slack text:test, but they do not cover application:slack or application:slack deployment.
  • Suggestion: Add the parameterized test shown in B1.

Test regressions

None found. No test files were deleted, no skips/xfails were added, and the changed assertions match the intentional simplified CLI contract.

CI failures

None found.

  • CI status: Test passed for head 0424c0d715d612d5cece99c54eaf5500096b61da in GitHub Actions run 26988708260, job 79643966782.

  • Skipped jobs: Publish to PyPI was skipped, not failed, in job 79643990644.

  • Local verification:

    uv run pytest tests/test_cli.py
    # 38 passed in 0.15s
    
    CTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytest
    # 54 passed in 0.30s
    
    git diff --check 7b5cb54fe2768f70fe8dd745688e4f3199d825f3...fix/search-fetch-signaling-10
    # no output
    

Unresolved PR comments

None found.

The previous top-level review comments are addressed in the current head: lowercase and / or, multi-word text, and repeated text: filters now have negative/positive coverage. The inline comment at discussion_r3359700921 is a question ("lmk what you think") on the help text, not a requested code change.

Positive observations

  • The current help text is now aligned around a single explicit text:<term> filter.
  • The CLI rejects invalid app filters, repeated app filters, grouped app filters, uppercase boolean clauses, quoted text, parenthesized text, shell-stripped multi-word text, bare continuation terms, and repeated text: filters before calling Client.search().
  • Unresolved document fetches now return exit code 1 in both text and JSON modes.
  • CI and local tests are green on the updated head.

Recommendations

  1. Fix B1 and add T1 before merging so application-scoped searches fully enforce the documented application:<app> text:<term> contract.
  2. Keep the existing regression tests for lowercase boolean words, multi-word text rejection, and repeated text: rejection.
  3. Separately from this PR, update the GitHub Actions versions flagged by the successful CI run's Node.js 20 deprecation warning before GitHub's 2026 cutoff dates.

Final verdict

Verdict: Request follow-up

B1/T1 is a medium validation gap on the search path this PR is tightening. CI is green and the earlier findings are fixed, but app-scoped searches without text:<term> should be rejected before merge.

@juanpabloguerra16

Copy link
Copy Markdown
Contributor Author

PR Review: fix/search-fetch-signaling-10

PR: #11 Improve CLI search and fetch signaling
Base: main at 7b5cb54
Head: d73caaba74d80877cfe3662b915d19f47e4e9acf
Commits: 7
Files changed: 3

Summary

This PR narrows CLI search syntax to a leading application:<app> plus one backend-supported text:<term> shape, rejects ambiguous DSL forms locally, updates README/help examples, and fixes unresolved fetch results so they exit non-zero. The previously raised validation gaps are now addressed: lowercase and / or terms are allowed, multi-word text values are rejected, repeated text: filters are rejected, and app-scoped searches now require one text:<term> filter. Overall risk is low after the latest commit.

Change inventory

File Change type Subsystem Risk
README.md modified Docs low
src/ctxd/cli.py modified CLI low
tests/test_cli.py modified Tests low

Apparent intent from commits:

  • 1a1c54f improves CLI search/fetch signaling.
  • f641625 simplifies CLI search query guidance.
  • 090dfed rejects complex text search forms.
  • e1b50ec allows lowercase boolean words in search text.
  • d03033d rejects unsupported multi-word text filters.
  • 0424c0d rejects repeated text search filters.
  • d73caab requires a text filter for app-scoped search.

Findings

Bugs

None found.

Design gaps

None found.

Side effects & risks

None found.

Edge cases & race conditions

None found.

Dead code

None found. The changed private helpers are reachable through main():

  • _normalize_search_query() and _validate_search_query() are called at src/ctxd/cli.py:35-36.
  • _split_search_query(), _validate_raw_text_filters(), _validate_application_filters(), _validate_boolean_operators(), _validate_text_filters(), and _validate_application_scoped_search_shape() are called from _validate_search_query() at src/ctxd/cli.py:171-177.
  • _payload_has_error() is called from JSON output at src/ctxd/cli.py:371.
  • _document_payload_is_unresolved() is called from _payload_has_error() at src/ctxd/cli.py:417.

Test coverage gaps

None 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 and / or terms, quoted text, parenthesized text, shell-stripped multi-word text, continuation terms, repeated text filters, app-scoped searches without text filters, valid leading app filters, payload errors, and unresolved fetch results.

Test regressions

None found. No test files were deleted, no skips/xfails were added, and the assertion changes match the intentional simplified CLI contract.

CI failures

None found.

  • CI status: Test passed for head d73caaba74d80877cfe3662b915d19f47e4e9acf in GitHub Actions run 26988865380, job 79644458291.

  • Skipped jobs: Publish to PyPI was skipped, not failed, in job 79644479971.

  • Local verification:

    uv run pytest tests/test_cli.py
    # 40 passed in 0.14s
    
    CTXD_CONFIG_PATH=/tmp/ctxd-test-config-missing.json env -u CTXD_API_KEY uv run pytest
    # 56 passed in 0.24s
    
    git diff --check 7b5cb54fe2768f70fe8dd745688e4f3199d825f3...fix/search-fetch-signaling-10
    # no output
    

Unresolved PR comments

None found.

The previous top-level review comments are addressed in the current head: lowercase and / or, multi-word text, repeated text: filters, and app-scoped searches without text:<term> now have coverage. The inline comment at discussion_r3359700921 is a question ("lmk what you think") on the help text, not a requested code change.

Positive observations

  • The CLI now fails locally before calling Client.search() for the ambiguous forms called out in the PR description.
  • Search validation is backed by targeted negative tests rather than relying on backend parse errors.
  • Unresolved document fetches now return exit code 1 in both text and JSON modes.
  • README and CLI help examples now consistently show the supported leading-application query shape.

Recommendations

  1. Merge after the normal human review gate; no blocking follow-up remains from this pass.
  2. Separately from this PR, update the GitHub Actions versions flagged by the successful CI run's Node.js 20 deprecation warning before GitHub's 2026 cutoff dates.

Final verdict

Verdict: 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.

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.

search/fetch: silent & misleading error/semantics signaling (DSL forms, multi-app filters, result cap, exit codes)

1 participant