Skip to content

Conversation

@cboone
Copy link
Owner

@cboone cboone commented Feb 9, 2026

  • docs: add plan to fix bugs, harden tests, and improve search UX
  • docs: fix plan counts and clarify live scrut strategy
  • fix: fix text search, harden tests, and improve search UX
  • test: exercise SearchEmails snippet path and harden live checks

Covers two bugs (broken text search, scrut test environment leakage),
four functional improvements (search pagination/sort/unread, mailbox
caching), two UX improvements (sort syntax flexibility, better error
messages), and three test improvements (environment isolation,
success-path tests, text format tests).
Fix broken text search by correcting the SearchSnippet/get JMAP result
reference to point at Email/query (path /ids) instead of Email/get
(path /list/*/id). Add --offset, --sort, and --unread flags to the
search command for parity with list. Accept colon syntax in --sort
(e.g. receivedAt:asc). Cache mailbox list per Client instance to
reduce redundant API calls. Improve JMAP error messages to include the
method name. Isolate scrut tests from ambient environment variables.
Add opt-in live integration tests and unit test for snippet reference.
Copilot AI review requested due to automatic review settings February 9, 2026 02:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes jm search text-query behavior, expands search UX capabilities (offset/sort/unread), and hardens the CLI scrut suite by isolating environment-dependent configuration. It also introduces an opt-in live integration scrut suite and updates documentation accordingly.

Changes:

  • Fix SearchSnippet/get JMAP result referencing for text search, and add unit coverage for that request shape.
  • Add search pagination/sort/unread support (flags + client implementation), plus improved search method-error messaging.
  • Make CLI scrut tests deterministic by unsetting JMAP_* env vars; add opt-in tests/live.md and split make test-cli vs make test-cli-live.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/client/email.go Fix snippet reference to Email/query /ids; add offset/sort/unread; improve method-error context; set result offset.
internal/client/email_test.go Add unit test ensuring SearchSnippet/get references Email/query /ids and populates snippets.
cmd/search.go Add --offset, --sort, --unread flags and validation; reuse parseSort.
cmd/list.go Extend parseSort to accept colon syntax (e.g. from:asc).
cmd/list_test.go Add tests for colon sort syntax.
internal/client/mailbox.go Cache mailbox list for the lifetime of a Client instance.
internal/client/client.go Add per-client mailbox cache storage and a doFunc hook to enable request capture in unit tests.
tests/errors.md Unset JMAP_* env vars for auth-failure expectations.
tests/flags.md Unset JMAP_* env vars for auth-failure expectations.
tests/arguments.md Unset JMAP_* env vars for auth-failure expectations.
tests/help.md Update search help snapshot for new flags.
tests/live.md Add opt-in live scrut tests for success paths, snippets, and text output.
Makefile Split deterministic scrut runs (test-cli) from opt-in live runs (test-cli-live).
docs/CLI-REFERENCE.md Document new search flags and colon sort syntax.
docs/plans/done/2026-02-08-fix-bugs-and-improve-search-tests-ux.md Add completed plan/design notes reflecting the implemented changes.
docs/plans/todo/dapper-sauteeing-taco.md Remove obsolete TODO plan.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add HOME=/nonexistent to env commands in error, flag, and argument tests
so that ~/.config/jm/config.yaml cannot leak a token or other settings
into the test environment.

Add --format json to live test subshells that parse output with
python3 json.load, ensuring deterministic behavior regardless of
JMAP_FORMAT env var or config file.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

cmd/list.go:68

  • With the new colon-to-space normalization, inputs like receivedAt:foo will now be treated as field=receivedAt with an unrecognized direction silently falling back to descending. Consider validating the direction token when present (accept only asc/desc, error otherwise) so typos don’t change sort behavior silently.
func parseSort(s string) (field string, ascending bool, err error) {
	s = strings.ReplaceAll(s, ":", " ")
	parts := strings.Fields(s)
	field = "receivedAt"
	ascending = false


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Each live test block now includes inline precondition checks
(JMAP_LIVE_TESTS and JMAP_TOKEN) so blocks properly skip when
conditions are unmet, since scrut exit 80 is block-scoped.
Commands asserting JSON output now pass --format json explicitly
to stay deterministic regardless of user config.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

cmd/list.go:79

  • parseSort treats any second token other than asc as descending (including invalid values). This conflicts with the CLI help/docs that imply only asc/desc are accepted, and can hide typos like --sort "receivedAt des". Consider validating the direction token explicitly (accept only asc/desc, error otherwise) so users get immediate feedback.
func parseSort(s string) (field string, ascending bool, err error) {
	s = strings.ReplaceAll(s, ":", " ")
	parts := strings.Fields(s)
	field = "receivedAt"
	ascending = false

	if len(parts) >= 1 {
		normalized, ok := validSortFields[strings.ToLower(parts[0])]
		if !ok {
			return "", false, fmt.Errorf("unsupported sort field %q", parts[0])
		}
		field = normalized
	}
	if len(parts) >= 2 && strings.EqualFold(parts[1], "asc") {
		ascending = true
	}
	return field, ascending, nil

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Replace redundant standalone precondition scrut blocks in live tests
with prose explaining the per-block guard pattern, since scrut exit 80
is block-scoped.

Tighten parseSort to reject invalid sort direction values instead of
silently falling back to descending.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cboone cboone merged commit e5ec80c into main Feb 9, 2026
7 checks passed
@cboone cboone deleted the feature/make-it-work branch February 9, 2026 16:10
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