Add --valuable-results to search feedback - #164
Open
dtaivpp wants to merge 2 commits into
Open
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
Wire the API's new valuableResultPositions field through both feedback commands so agents can attribute usefulness to specific data.web results by 1-indexed position. Skills now surface positions in jq extraction snippets and require exhaustive position marking (unlisted results count as not useful), with valuableSources reserved for URLs outside data.web. Requires an API with position-based search feedback support (firecrawl/firecrawl#4109); the field is only sent when the flag is provided. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Search results come back grouped — data.web, data.images, data.news — and
each group is numbered from 1 independently, so a bare position only ever
meant "web" and could not name a news or image result at all. That matters
most for those groups: news and image results have optional URLs, so
--valuable-sources cannot reliably address them either.
Takes "source:position" pairs (e.g. "web:1,news:2") or a JSON array of
{source, position, reason} entries, matching the API's valuableResults
field (firecrawl/firecrawl#4109). The source is always required — "web:1"
and "news:1" are different results.
Also reverts the generic parsePositiveIntArrayArg helper this branch had
extracted from parsePageNumbersArg; nothing shares it now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/commands/feedback.ts">
<violation number="1" location="src/commands/feedback.ts:269">
P2: The `feedback` command serves `search`, `scrape`, `parse`, and `map` endpoints, but `valuableResults` is only meaningful for search results, whose positions index into `web`/`images`/`news` groups. This change forwards `valuableResults` to every endpoint with no guard, so a user passing `--valuable-results web:1` on a scrape/parse/map job sends a body referencing groups that do not exist for that job, producing an API error. The CLI help already labels the flag "Search only"; enforce that by rejecting `valuableResults` when the endpoint is not `search`.</violation>
<violation number="2" location="src/commands/feedback.ts:269">
P1: The PR goal is to forward the API's `valuableResultPositions` field by raising a new `--valuable-result-positions` flag that accepts bare 1-indexed positions ("1,3" or [1,3]), parsed like `--page-numbers`. This change instead reuses the existing `--valuable-results` flag and `parseValuableResultsArg`, which only accepts `source:position` objects (e.g. `web:1`) and rejects bare positions, then forwards them as the field `valuableResults`. As a result the stated capability is not delivered: bare-position input errors out and the intended `valuableResultPositions` field is never sent. Note this does match the in-repo README/help text (which document `source:position`), so the code and the PR description conflict; confirm which contract the API expects and align the flag, parser, and forwarded field name.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Sorry, something went wrong.
Comment thread
src/commands/feedback.ts
| ['tags', normalizeList(options.tags)], | ||
| ['note', options.note], | ||
| ['valuableSources', options.valuableSources], | ||
| ['valuableResults', options.valuableResults], |
Copy link
Copy Markdown
There was a problem hiding this comment.
P1: The PR goal is to forward the API's valuableResultPositions field by raising a new --valuable-result-positions flag that accepts bare 1-indexed positions ("1,3" or [1,3]), parsed like --page-numbers. This change instead reuses the existing --valuable-results flag and parseValuableResultsArg, which only accepts source:position objects (e.g. web:1) and rejects bare positions, then forwards them as the field valuableResults. As a result the stated capability is not delivered: bare-position input errors out and the intended valuableResultPositions field is never sent. Note this does match the in-repo README/help text (which document source:position), so the code and the PR description conflict; confirm which contract the API expects and align the flag, parser, and forwarded field name.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At src/commands/feedback.ts, line 269:
<comment>The PR goal is to forward the API's `valuableResultPositions` field by raising a new `--valuable-result-positions` flag that accepts bare 1-indexed positions ("1,3" or [1,3]), parsed like `--page-numbers`. This change instead reuses the existing `--valuable-results` flag and `parseValuableResultsArg`, which only accepts `source:position` objects (e.g. `web:1`) and rejects bare positions, then forwards them as the field `valuableResults`. As a result the stated capability is not delivered: bare-position input errors out and the intended `valuableResultPositions` field is never sent. Note this does match the in-repo README/help text (which document `source:position`), so the code and the PR description conflict; confirm which contract the API expects and align the flag, parser, and forwarded field name.</comment>
<file context>
@@ -261,6 +266,7 @@ export async function executeEndpointFeedback(
['tags', normalizeList(options.tags)],
['note', options.note],
['valuableSources', options.valuableSources],
+ ['valuableResults', options.valuableResults],
['missingContent', options.missingContent],
['querySuggestions', options.querySuggestions],
</file context>
Sorry, something went wrong.
Comment thread
src/commands/feedback.ts
| ['tags', normalizeList(options.tags)], | ||
| ['note', options.note], | ||
| ['valuableSources', options.valuableSources], | ||
| ['valuableResults', options.valuableResults], |
Copy link
Copy Markdown
There was a problem hiding this comment.
P2: The feedback command serves search, scrape, parse, and map endpoints, but valuableResults is only meaningful for search results, whose positions index into web/images/news groups. This change forwards valuableResults to every endpoint with no guard, so a user passing --valuable-results web:1 on a scrape/parse/map job sends a body referencing groups that do not exist for that job, producing an API error. The CLI help already labels the flag "Search only"; enforce that by rejecting valuableResults when the endpoint is not search.
Prompt for AI agentsCheck if this issue is valid — if so, understand the root cause and fix it. At src/commands/feedback.ts, line 269:
<comment>The `feedback` command serves `search`, `scrape`, `parse`, and `map` endpoints, but `valuableResults` is only meaningful for search results, whose positions index into `web`/`images`/`news` groups. This change forwards `valuableResults` to every endpoint with no guard, so a user passing `--valuable-results web:1` on a scrape/parse/map job sends a body referencing groups that do not exist for that job, producing an API error. The CLI help already labels the flag "Search only"; enforce that by rejecting `valuableResults` when the endpoint is not `search`.</comment>
<file context>
@@ -261,6 +266,7 @@ export async function executeEndpointFeedback(
['tags', normalizeList(options.tags)],
['note', options.note],
['valuableSources', options.valuableSources],
+ ['valuableResults', options.valuableResults],
['missingContent', options.missingContent],
['querySuggestions', options.querySuggestions],
</file context>
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why the source is required
Search results come back grouped — data.web, data.images, data.news — and each group is numbered from 1 independently. A bare position could only ever mean "web", so news and image results were unreachable. That matters most for those groups: their url fields are optional, so --valuable-sources can't reliably address them either. web:1 and news:1 are different results, and the parser rejects a bare 1,3 rather than guessing.
Also
Reverts the generic parsePositiveIntArrayArg helper this branch had extracted from parsePageNumbersArg — nothing shares it now that positions aren't a plain int array.
Tests
🤖 Generated with Claude Code