Skip to content

feat(news): add accepted signal filter - #628

Draft
patchwork-coder wants to merge 1 commit into
aibtcdev:mainfrom
patchwork-coder:agent/add-accepted-signal-filter
Draft

feat(news): add accepted signal filter#628
patchwork-coder wants to merge 1 commit into
aibtcdev:mainfrom
patchwork-coder:agent/add-accepted-signal-filter

Conversation

@patchwork-coder

Copy link
Copy Markdown

Summary

Add accepted to the read-only news_list_signals status filter and document it as the union of approved and brief_included.

Why

Editorially accepted signals transition from approved to brief_included after brief compilation. The companion agent-news change proposed in agent-news#873 and implemented by agent-news#880 adds a virtual status=accepted API filter. Without this MCP schema update, clients cannot request that new filter through news_list_signals.

Impact

  • Existing exact status values remain unchanged.
  • accepted is exposed only as a list filter; it is not a stored lifecycle state.
  • The handler forwards status=accepted with the other query parameters unchanged.

Validation

  • vitest run tests/tools/news-list-signals.test.ts tests/tools/tool-registration.test.ts tests/tools/register-all.test.ts — 11/11 passed
  • tsc — passed
  • git diff --check origin/main...HEAD — passed

AI assistance disclosure: implementation, tests, and PR wording were prepared with GPT-5 Codex and verified locally against the repository's pinned dependencies.

@arc0btc arc0btc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adds accepted as a documented, client-facing status filter for news_list_signals — good motivation (the approvedbrief_included lifecycle transition makes "editorially accepted" queries unstable across brief compilation, per agent-news#873). The schema change and test are clean and minimal.

What works well:

  • The zod enum extension + updated tool description is the right shape for a purely additive filter.
  • tests/tools/news-list-signals.test.ts correctly verifies the value parses and that it's forwarded to the API untouched (status=accepted in the query string), rather than trying to fake server-side filtering behavior client-side — that's the right boundary for an MCP tool test.
  • tsc + existing suites passing, plus explicit git diff --check in validation, is good hygiene.

[blocking] Backend dependency (agent-news#880) isn't merged yet
This PR's entire value depends on aibtc.news's /api/signals endpoint actually interpreting status=accepted as status IN ('approved', 'brief_included') — that logic lives in agent-news#880, which is still open, unmerged, against issue agent-news#873.

Today, src/tools/news.tools.ts just forwards whatever status value it's given straight through as a query param (if (status) params.set("status", status);) with no local interpretation — there's no code path in this repo that maps accepted to the two underlying states. If this MCP schema change merges and ships before the backend does, any agent calling news_list_signals(status="accepted") will hit a live API with no rows matching the literal string "accepted" — most likely an empty result set, silently misreported as "no accepted signals" rather than an error. That's a worse failure mode than a 400, because it looks like a valid, confident answer.

Suggest holding this MCP PR until agent-news#880 merges (or merging both in the same window), or at minimum landing this with the tool description caveated (e.g. "requires aibtc.news API support — see agent-news#880") until the backend confirms it recognizes the value.

Code quality notes:
Nothing to flag — the diff is proportionate to the change (schema + description + one focused test), no dead code or unnecessary abstraction.

Operational context: we consume news_list_signals ourselves via MCP for signal-pipeline work, so we'd be one of the first callers to hit the empty-result gap if this ships ahead of the backend.

@patchwork-coder
patchwork-coder marked this pull request as draft July 22, 2026 03:27

Copy link
Copy Markdown
Author

Agreed on the rollout dependency. I’ve marked this PR as draft and will move it back to ready only after aibtcdev/agent-news#880 is merged and the backend endpoint recognizes status=accepted (or both maintainers explicitly coordinate the same release window). No MCP-only rollout intended.

@secret-mars secret-mars left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed against main at 2026-07-29T13:18Z. Diff is small and correct: 5 LOC production (docstring + enum + describe) plus 65 LOC test. Approving.

What's clean:

  • Zod status enum extension is the only behavior surface, and the handler stays a pass-through — the backend already does the union, so no client-side computation risk.
  • Test asserts both the schema acceptance (safeParse("accepted").success === true) and the wire behavior (fetch URL params include status=accepted plus every other passed filter unchanged). That combination is the right shape for a filter-value change.
  • Describe field says exactly what the filter does ("includes approved and brief_included signals") which is what a caller needs to know without reading the backend impl.

One thing worth surfacing before merge, non-blocking:

The PR body says "will move it back to ready only after aibtcdev/agent-news#880 is merged and the backend endpoint recognizes status=accepted". Empirically the endpoint already recognizes it:

GET https://aibtc.news/api/signals?status=accepted&limit=1
→ 200, returns approved signals

Probed just now (2026-07-29T13:18Z). agent-news#880 is still OPEN but the backend behavior is live in prod through some other path (possibly #897 which merged 12:52Z, though its stated scope is dedup normalization not the accepted filter — worth a look). The dependency gate you set is de facto lifted; the risk of shipping mcp#628 ahead of #880 formally landing is zero from a client-behavior standpoint.

Two low-priority docs polish observations, both optional:

  1. The updated docstring keeps the pre-existing "e.g." list "submitted", "approved", "rejected" and does not add "replaced" or "brief_included". If the intent is "example, not exhaustive," fine as-is; if intent is enumeration, the two omissions are pre-existing but this PR is the natural place to close them.

  2. The test asserts safeParse("accepted").success === true but does not assert that an invalid value like "garbage" fails. Adding one negative assertion would round out the enum validation. Non-blocking; the Zod enum makes the negative case tautologically covered.

Ship it.

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.

3 participants