-
Notifications
You must be signed in to change notification settings - Fork 18
feat(code): Suggest experiment opportunities in setup discovery #2109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,21 @@ | ||
| import { getAuthenticatedClient } from "@features/auth/hooks/authClient"; | ||
| import { fetchAuthState } from "@features/auth/hooks/authQueries"; | ||
| import { DISCOVERY_PROMPT } from "@features/setup/prompts"; | ||
| import { buildDiscoveryPrompt } from "@features/setup/prompts"; | ||
| import { useSetupStore } from "@features/setup/stores/setupStore"; | ||
| import { | ||
| buildTaskDiscoverySchema, | ||
| type DiscoveredTask, | ||
| TASK_DISCOVERY_JSON_SCHEMA, | ||
| } from "@features/setup/types"; | ||
| import { trpcClient } from "@renderer/trpc/client"; | ||
| import { EXPERIMENT_SUGGESTIONS_FLAG } from "@shared/constants"; | ||
| import { isTerminalStatus, type Task } from "@shared/types"; | ||
| import { ANALYTICS_EVENTS } from "@shared/types/analytics"; | ||
| import { getCloudUrlFromRegion } from "@shared/utils/urls"; | ||
| import { captureException, track } from "@utils/analytics"; | ||
| import { | ||
| captureException, | ||
| isFeatureFlagEnabled, | ||
| track, | ||
| } from "@utils/analytics"; | ||
| import { logger } from "@utils/logger"; | ||
| import { injectable } from "inversify"; | ||
|
|
||
|
|
@@ -349,10 +354,16 @@ export class SetupRunService { | |
| return; | ||
| } | ||
|
|
||
| const includeExperiments = | ||
| isFeatureFlagEnabled(EXPERIMENT_SUGGESTIONS_FLAG) || | ||
| import.meta.env.DEV; | ||
|
Comment on lines
+357
to
+359
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/setup/services/setupRunService.ts
Line: 357-359
Comment:
**`import.meta.env.DEV` makes the flag bypass un-testable in dev**
`isFeatureFlagEnabled(EXPERIMENT_SUGGESTIONS_FLAG) || import.meta.env.DEV` means every developer running a local build always gets experiment suggestions, with no way to exercise the flag-off code path without a production build. If someone wants to verify the fallback behaviour (e.g. confirming that `experiment` cards surfaced during a flag-on session still render after the flag is turned off), they cannot do so from a normal dev environment. Consider whether this was intentional or whether a more explicit local override (e.g. a query-param or an env var) would give more control during testing.
How can I resolve this? If you propose a fix, please make it concise. |
||
| const discoveryPrompt = buildDiscoveryPrompt({ includeExperiments }); | ||
| const discoverySchema = buildTaskDiscoverySchema({ includeExperiments }); | ||
|
|
||
| const task = (await client.createTask({ | ||
| title: "Discover first tasks", | ||
| description: DISCOVERY_PROMPT, | ||
| json_schema: TASK_DISCOVERY_JSON_SCHEMA as Record<string, unknown>, | ||
| description: discoveryPrompt, | ||
| json_schema: discoverySchema, | ||
| })) as unknown as Task; | ||
| if (abort.signal.aborted) return; | ||
|
|
||
|
|
@@ -375,14 +386,14 @@ export class SetupRunService { | |
| apiHost, | ||
| projectId, | ||
| permissionMode: "bypassPermissions", | ||
| jsonSchema: TASK_DISCOVERY_JSON_SCHEMA as Record<string, unknown>, | ||
| jsonSchema: discoverySchema, | ||
| }); | ||
| if (abort.signal.aborted) return; | ||
|
|
||
| trpcClient.agent.prompt | ||
| .mutate({ | ||
| sessionId: taskRun.id, | ||
| prompt: [{ type: "text", text: DISCOVERY_PROMPT }], | ||
| prompt: [{ type: "text", text: discoveryPrompt }], | ||
| }) | ||
| .catch((err) => { | ||
| log.error("Failed to send discovery prompt", { error: err }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BASE_ALLOWED_CATEGORIESinprompts.tsandBASE_CATEGORY_ENUMintypes.tsenumerate exactly the same nine base categories. If a new category is added to the schema in the future, both constants must be updated in sync — a gap that will inevitably be missed. The prompt's allowed-categories string should be derived from the schema's enum array (e.g. exportBASE_CATEGORY_ENUMfromtypes.tsand call.join(", ")here) so there is a single source of truth.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grok is this true? 🤔