Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 4/5
search-feedback-objective.test.tstreats a missing--objectiveas success, which could let the required-flag behavior go unenforced. Change the test to expect rejection when the flag is omitted.src/index.tsduplicates thedetailvalidator inalexandria-feedback.ts, so the two copies could drift. Reuse the existing validator.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/index.ts">
<violation number="1" location="src/index.ts:1404">
P3: `searchFeedbackObjective` duplicates the identical `detail` validator already defined in src/commands/alexandria-feedback.ts (same trim, same 2000-char check, same 'Use 1–2000 characters.' message and it's used for the same `--objective` purpose). Extract a shared text-validator helper into a utils module and reuse it in both places so the limit/message don't drift.</violation>
</file>
<file name="src/__tests__/search-feedback-objective.test.ts">
<violation number="1" location="src/__tests__/search-feedback-objective.test.ts:84">
P2: This test asserts that omitting `--objective` succeeds (silently no-op via `--silent` if the server rejects it). That contradicts the PR's stated intent of "Adds a required `--objective` flag," and the implementation uses a plain `.option(...)` (not `.requiredOption`), so the CLI never enforces it. If the backend now requires objective, agents that omit it (e.g. existing `--silent &` skills feedback) will send payloads the API rejects, and because `--silent` exits 0 on failure the error is swallowed. Make the flag actually required with `.requiredOption(...)` (and reject omission in this test), or update the PR description/skill doc to say it is optional-on-the-CLI.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| }); | ||
| }); | ||
|
|
||
| it('still sends feedback without an objective', async () => { |
There was a problem hiding this comment.
P2: This test asserts that omitting --objective succeeds (silently no-op via --silent if the server rejects it). That contradicts the PR's stated intent of "Adds a required --objective flag," and the implementation uses a plain .option(...) (not .requiredOption), so the CLI never enforces it. If the backend now requires objective, agents that omit it (e.g. existing --silent & skills feedback) will send payloads the API rejects, and because --silent exits 0 on failure the error is swallowed. Make the flag actually required with .requiredOption(...) (and reject omission in this test), or update the PR description/skill doc to say it is optional-on-the-CLI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/__tests__/search-feedback-objective.test.ts, line 84:
<comment>This test asserts that omitting `--objective` succeeds (silently no-op via `--silent` if the server rejects it). That contradicts the PR's stated intent of "Adds a required `--objective` flag," and the implementation uses a plain `.option(...)` (not `.requiredOption`), so the CLI never enforces it. If the backend now requires objective, agents that omit it (e.g. existing `--silent &` skills feedback) will send payloads the API rejects, and because `--silent` exits 0 on failure the error is swallowed. Make the flag actually required with `.requiredOption(...)` (and reject omission in this test), or update the PR description/skill doc to say it is optional-on-the-CLI.</comment>
<file context>
@@ -0,0 +1,101 @@
+ });
+});
+
+it('still sends feedback without an objective', async () => {
+ const index = feedbackArgs.indexOf('--objective');
+ const withoutObjective = feedbackArgs.filter(
</file context>
| return researchCmd; | ||
| } | ||
|
|
||
| function searchFeedbackObjective(value: string): string { |
There was a problem hiding this comment.
P3: searchFeedbackObjective duplicates the identical detail validator already defined in src/commands/alexandria-feedback.ts (same trim, same 2000-char check, same 'Use 1–2000 characters.' message and it's used for the same --objective purpose). Extract a shared text-validator helper into a utils module and reuse it in both places so the limit/message don't drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/index.ts, line 1404:
<comment>`searchFeedbackObjective` duplicates the identical `detail` validator already defined in src/commands/alexandria-feedback.ts (same trim, same 2000-char check, same 'Use 1–2000 characters.' message and it's used for the same `--objective` purpose). Extract a shared text-validator helper into a utils module and reuse it in both places so the limit/message don't drift.</comment>
<file context>
@@ -1401,6 +1401,13 @@ Examples:
return researchCmd;
}
+function searchFeedbackObjective(value: string): string {
+ const text = value.trim();
+ if (!text || text.length > 2000)
</file context>
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Confidence score: 4/5
search-feedback-objective.test.tscan exit successfully without sending a request when either disable flag is set in the contributor’s environment. Clear those flags in the child process environment.src/index.ts’s >2000 rejection branch insearchFeedbackObjectiveis untested, so regressions in the upper-bound check could go unnoticed. Add a test for that boundary.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/__tests__/search-feedback-objective.test.ts">
<violation number="1" location="src/__tests__/search-feedback-objective.test.ts:46">
P3: The child process inherits `process.env`, so a contributor with `FIRECRAWL_NO_SEARCH_FEEDBACK` or `FIRECRAWL_DISABLE_SEARCH_FEEDBACK` set locally gets the CLI exiting 0 without sending any request, and the first test fails on `requests` being empty (see `isSearchFeedbackDisabledLocally` in src/commands/search-feedback.ts). Neutralize both opt-out vars in the child env so the test is deterministic.</violation>
</file>
<file name="src/index.ts">
<violation number="1" location="src/index.ts:1406">
P3: The >2000 rejection branch of `searchFeedbackObjective` has no test coverage. `src/__tests__/search-feedback-objective.test.ts` exercises only the missing and blank cases, so the newly added upper-bound check is untested.</violation>
</file>
Shadow auto-approve: would auto-approve with 2 open P3 issues. Adds an optional --objective flag to the search-feedback CLI with trimming/validation, forwarding in the feedback request, skill documentation, and tests; existing behavior without the flag is preserved.
Fix all with cubic | Re-trigger cubic
| ...(await exec(process.execPath, ['dist/index.js', ...args], { | ||
| timeout: 10000, | ||
| env: { | ||
| ...process.env, |
There was a problem hiding this comment.
P3: The child process inherits process.env, so a contributor with FIRECRAWL_NO_SEARCH_FEEDBACK or FIRECRAWL_DISABLE_SEARCH_FEEDBACK set locally gets the CLI exiting 0 without sending any request, and the first test fails on requests being empty (see isSearchFeedbackDisabledLocally in src/commands/search-feedback.ts). Neutralize both opt-out vars in the child env so the test is deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/__tests__/search-feedback-objective.test.ts, line 46:
<comment>The child process inherits `process.env`, so a contributor with `FIRECRAWL_NO_SEARCH_FEEDBACK` or `FIRECRAWL_DISABLE_SEARCH_FEEDBACK` set locally gets the CLI exiting 0 without sending any request, and the first test fails on `requests` being empty (see `isSearchFeedbackDisabledLocally` in src/commands/search-feedback.ts). Neutralize both opt-out vars in the child env so the test is deterministic.</comment>
<file context>
@@ -0,0 +1,96 @@
+ ...(await exec(process.execPath, ['dist/index.js', ...args], {
+ timeout: 10000,
+ env: {
+ ...process.env,
+ HOME: home,
+ USERPROFILE: home,
</file context>
|
|
||
| function searchFeedbackObjective(value: string): string { | ||
| const text = value.trim(); | ||
| if (!text || text.length > 2000) |
There was a problem hiding this comment.
P3: The >2000 rejection branch of searchFeedbackObjective has no test coverage. src/__tests__/search-feedback-objective.test.ts exercises only the missing and blank cases, so the newly added upper-bound check is untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/index.ts, line 1406:
<comment>The >2000 rejection branch of `searchFeedbackObjective` has no test coverage. `src/__tests__/search-feedback-objective.test.ts` exercises only the missing and blank cases, so the newly added upper-bound check is untested.</comment>
<file context>
@@ -1401,6 +1401,13 @@ Examples:
+function searchFeedbackObjective(value: string): string {
+ const text = value.trim();
+ if (!text || text.length > 2000)
+ throw new InvalidArgumentError('Use 1–2000 characters.');
+ return text;
</file context>
|
Closing: not proceeding with this change for now. |
1 participant
Summary
Adds a required
--objectiveflag tosearch-feedback: the underlying goal behind the search. The search skill's feedback example now includes it.Requires the API change to be deployed first.
Checks
Typecheck and full test suite (660 tests) passed.
Summary by cubic
Adds an optional
--objectiveflag tosearch-feedbackso feedback can capture the underlying goal behind the search, not just the query terms.--objective.objectiveto be deployed before this ships.Written for commit 45318d9. Summary will update on new commits.