AMP-150628 fix bugs and inconsistencies in BDD feature files#9
Merged
kaiapeacock-eng merged 1 commit intomainfrom Mar 16, 2026
Merged
AMP-150628 fix bugs and inconsistencies in BDD feature files#9kaiapeacock-eng merged 1 commit intomainfrom
kaiapeacock-eng merged 1 commit intomainfrom
Conversation
- Fix typo: "detailsy" → "details" in 01-top-level-commands.feature - Fix broken step "When I should go through the Data Setup flow" → "When the Data Setup check runs" in 02-wizard-flow.feature - Add missing "When" step to settings override scenario: "When the agent is about to start" (matches where agent-runner.ts checks for blocking Claude settings overrides) - Standardize slash command step phrasing in 09-slash-commands.feature: "When I run" → "When I enter the slash command" to match the implemented step definition in wizard-overlays.steps.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kaiapeacock-eng
approved these changes
Mar 16, 2026
Collaborator
kaiapeacock-eng
left a comment
kaiapeacock-eng
left a comment
There was a problem hiding this comment.
don't worry about the checks failing, I'll get those in order soon
bird-m
added a commit
that referenced
this pull request
Apr 15, 2026
Review-panel #1 (Severe): Remove competing browser tab in requires_auth path. The requires_auth branch opened the backend's redirect URL via opn() then fell through to performAmplitudeAuth which opened a second browser tab with mismatched PKCE. Now we skip the backend URL and let performAmplitudeAuth handle the entire flow. Added TODO to evaluate using the backend URL in a follow-up. Review-panel #2 (Critical): Init feature flags in agent/CI paths so _headlessSignupEnabled can be true. Previously the flag was only set in the TUI interactive path, making the entire agent/CI headless signup block unreachable dead code. Review-panel #3 (Important): Pre-populate HeadlessSignupScreen from CLI --email/--full-name flags. Auto-submit if both are present. Review-panel #4 (Important): Redact email in agent NDJSON log output to match the redaction pattern in headless-signup.ts. Review-panel #7 (Important): Split fullName into first_name/last_name on first space before sending to provisioning endpoint. Review-panel #9 (Nit): Replace non-null assertions on headlessSignupEmail/headlessSignupFullName with an explicit guard. Review-panel #10 (Nit): Extract completeSignupTokenExchange into headless-signup.ts as a shared helper used by both the agent/CI and TUI code paths, reducing duplication and drift risk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bird-m
added a commit
that referenced
this pull request
Apr 16, 2026
Prevents the user from hanging indefinitely if the endpoint is
unresponsive. On timeout, returns { type: 'error', code: 'timeout' }
so the caller can fall back to browser OAuth.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bird-m
added a commit
that referenced
this pull request
Apr 16, 2026
Move completeAuth out of the authTask IIFE into src/utils/auth-complete.ts so both the interactive and non-interactive signup paths share one pipeline (fetch user, store token, update analytics, signal TUI). Drops the duplicated completeSignupTokenExchange. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
kelsonpw
added a commit
that referenced
this pull request
Apr 17, 2026
… picker) Batch of bird-m and Bugbot findings from PR #112: #1 claudeCodeMode default mismatch (medium, flagged 3×): addMCPServerToClientsStep silently defaulted to 'plugin' while mcp-installer.ts defaulted to 'mcp'. Non-TUI fallback via bin.ts could install the plugin with no user prompt. Aligned both to 'mcp' as the safer default — plugin is interactive-only now. #3 remove flow can't uninstall plugin (medium, flagged 2×): getInstalledClients only ever instantiated ClaudeCodeMCPClient. After plugin install removes the bare `amplitude` MCP entry, isServerInstalled returned false and `wizard mcp remove` silently skipped Claude Code. Added an explicit ClaudeCodePluginClient probe before falling through to the MCP check. #4 non-TUI `local` flag ignored for plugin path (low, Bugbot): addMCPServerToClientsStep now forces 'mcp' mode whenever local=true, matching the TUI's behavior. The plugin hardcodes the prod URL and can't serve localhost. #5 `as unknown as RawMCPClient[]` cast (nit, bird-m): resolveClientsForMode returns MCPClient[] directly; the local RawMCPClient interface was a holdover. Dropped the cast and removed the unused interface — install loop now type-checks against MCPClient directly. #6 older Claude CLIs fail opaquely (nit, bird-m): ClaudeCodePluginClient.isClientSupported now probes `claude plugin --help` in addition to `--version`. resolveClientsForMode is async and checks plugin support before swapping — older CLIs quietly keep ClaudeCodeMCPClient instead of failing during `marketplace add`. #7 single-Claude-Code hid plugin/MCP choice (medium, Bugbot): detected.length === 1 routed to Phase.Ask, which doesn't show the split picker. Now: if the lone detected tool is Claude Code and no escape hatch is set, route to Phase.Pick so the user sees plugin vs MCP rows. #8 resolveSelection misleading default (low, Bugbot): wantsPlugin || !wantsMcp ? 'plugin' : 'mcp' returned 'plugin' when the user unchecked both Claude Code rows. Simplified to wantsPlugin ? 'plugin' : 'mcp' — explicit semantics, still correct since downstream guards the Claude-Code-absent case. #9 Codex Windows detection (low, Bugbot): `command -v` is POSIX-only; Windows never matched. Use `where codex` on win32, `command -v codex` on POSIX. Take the first line since `where` may return multiple paths. Narrowed the bundled-app exclusion to macOS only (Conductor-specific). #10 multi-picker uncheck-all dead code (medium, Bugbot): MultiPickerMenu's Enter handler fell back to the focused row when selected was empty, so a user who unchecked every pre-selected row got one install instead of a skip. Now: if defaultSelected was provided, an empty set means deliberate — pass [] through to the caller. Also fixed lexicographic index sort → numeric. #? dev script env var at build-time (low, Bugbot): `AMPLITUDE_WIZARD_DEV=1 pnpm build` only scoped the var to the build subprocess, not the globally-linked binary. Removed from the `dev` script since it was ineffective there — `try` still sets it at runtime where it actually works. Addressed in comment, no code change: #2 non-atomic settings.json write — already replaced with `claude plugin marketplace add` CLI in commit 4679fc4. No direct file write remains. 974 tests pass, lint clean, smoke test passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
5 tasks
kelsonpw
added a commit
that referenced
this pull request
Apr 27, 2026
Bundles audit tasks #9 (brand IDs) and #10 (RunPhase as discriminated view). Adds AppId / WorkspaceId branded types via z.brand and exposes phase-narrowed session views (AuthenticatedSession / ConfiguredSession / RunningSession) plus type guards (isAuthenticated / isConfigured / isRunning) so callers can stop sprinkling defensive null checks. Branding addresses the org-data-mapping bug class (PR #62), where a workspace id and an app id are structurally `string`/`number` and the type system couldn't catch a mix-up. Brands are zero-cost at runtime but distinct nominally, with `toAppId()` / `toWorkspaceId()` helpers at parse boundaries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kelsonpw
added a commit
that referenced
this pull request
Apr 28, 2026
#244) * refactor(session): brand AppId/WorkspaceId and gate fields by RunPhase Bundles audit tasks #9 (brand IDs) and #10 (RunPhase as discriminated view). Adds AppId / WorkspaceId branded types via z.brand and exposes phase-narrowed session views (AuthenticatedSession / ConfiguredSession / RunningSession) plus type guards (isAuthenticated / isConfigured / isRunning) so callers can stop sprinkling defensive null checks. Branding addresses the org-data-mapping bug class (PR #62), where a workspace id and an app id are structurally `string`/`number` and the type system couldn't catch a mix-up. Brands are zero-cost at runtime but distinct nominally, with `toAppId()` / `toWorkspaceId()` helpers at parse boundaries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: use AppIdSchema.safeParse in tryToAppId and CliArgsSchema transform Applied via @cursor push command * fix(tui): pass null instead of toWorkspaceId('') on workspace clear setOrgAndWorkspace is called with `{ id: '', name: '' }` from several AuthScreen reset paths (stale-org clear, "Start Over", create-project fallback). Routing those through `toWorkspaceId('')` throws a ZodError because `WorkspaceIdSchema` requires `min(1)`. Collapse empty ids to `null` so the clear/reset semantics survive the new branding. Adds two regression tests covering the empty-id and non-empty-id paths. * fix(tui): apply empty-id guard to restoreSessionIds for consistency Bugbot flagged that restoreSessionIds called toWorkspaceId(fields.workspaceId) without the same empty-string guard added to setOrgAndWorkspace. No current caller passes an empty string here (CreateProjectScreen omits workspaceId, DataIngestionCheckScreen reads from API responses), but applying the guard consistently across both write paths removes the asymmetry and prevents future regressions. Adds two regression tests mirroring the setOrgAndWorkspace coverage. * fix(tui): collapse empty selectedOrgId to null on workspace clear Bugbot flagged that AuthScreen reset paths (Start Over, stale-org clear, create-project fallback) call setOrgAndWorkspace({ id: '', name: '' }, ...) which set selectedOrgId to '' without clearing credentials. Since isAuthenticated only checks `selectedOrgId !== null`, the guard returned true with a meaningless empty org id. Mirror the workspaceId behavior: collapse '' -> null so isAuthenticated correctly reports the session as un-orged after a reset. Updates the existing regression test to assert both fields. * fix(tui): mirror empty-orgId guard in restoreSessionIds Apply the same '' -> null collapse to restoreSessionIds.orgId that setOrgAndWorkspace just got. No current caller passes empty strings here, but keeping both write paths symmetric prevents a future caller from leaving an empty selectedOrgId that would slip past isAuthenticated. * fix(session): coerce empty/whitespace selectedWorkspaceId to null in checkpoint A whitespace-only selectedWorkspaceId in a checkpoint file slips past the truthy ternary guard and reaches `toWorkspaceId(...)`, which rejects via `z.string().min(1)` and throws — taking down checkpoint restore. Tighten the schema so empty / whitespace values are coerced to `null` before the ternary runs, keeping the guard a single source of truth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(session): cover isAuthenticated un-narrow after credentials cleared Adds a regression test for the narrowing-then-clearing path: a session narrowed via isAuthenticated() is updated to set credentials = null and the guard is re-checked. This locks in that the guard correctly reports false on the new session, mirroring the logout / token-expiry flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(session): brand credentials.appId so cross-id mix-ups can't sneak past TS PR #62's "org data mapping" bug came from raw numbers being passed where an app id was expected (or vice versa). The top-level `WizardSession.appId` and `selectedWorkspaceId` were already branded, but `credentials.appId` — the field that's actually constructed at every OAuth / API-key boundary — was left as plain `number`. That's the field most likely to carry a mis-shaped id into a downstream call. Brand it as `AppId | 0`, where `0` remains the "env not picked yet" sentinel. Add a `toCredentialAppId(value)` helper that validates raw input via `AppIdSchema.safeParse(...)` and falls back to `0` on failure, and route every construction site through it: - bin.ts (create-project flow) - src/lib/credential-resolution.ts (5 sites) - src/utils/setup-utils.ts (3 return sites + ProjectData type) - src/ui/tui/ink-ui.ts (re-validates at the InkUI store boundary) - src/ui/tui/screens/AuthScreen.tsx (3 picker / fetch sites) - src/ui/tui/screens/CreateProjectScreen.tsx (1 site) `agent-ui.ts:setCredentials()` is a pure NDJSON sink and keeps the public `appId: number` contract intact — branded values are still numbers at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
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
2 participants
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
detailsy→detailsin01-top-level-commands.featureWhen I should go through the Data Setup flow→When the Data Setup check runsin02-wizard-flow.featureWhen the agent is about to startstep to the settings override scenario (matches whereagent-runner.tschecks for blocking Claude settings overrides)09-slash-commands.feature:When I run→When I enter the slash commandto match the implemented step definition inwizard-overlays.steps.tsTest plan
pnpm test:bddpasses (no step definition mismatches introduced)🤖 Generated with Claude Code