Skip to content

Commit 1f92780

Browse files
authored
chore(agentic): improve mobile recipe validation and perps testability (#28974)
## **Description** This PR is limited to mobile app and mobile agentic validation improvements discovered while validating perps review flows on `mm-4`. Scope of this PR: - stabilize Perps modify-action selectors so live validation no longer depends on the accidental `undefined-*` selector path - tighten the mobile agentic recipe contract so invalid `wait_for.timeout` usage fails fast - make `validate-recipe.js` schema-validate before execution - copy screenshots into the passed artifact directory so validation proof stays with the run artifacts - document the canonical mobile agentic timing fields in `scripts/perps/agentic/README.md` Compatibility note: - default recipe artifact behavior is unchanged - only explicit `--artifacts-dir` runs now treat the provided path as the artifact root, so screenshots and run artifacts stay together under the requested directory Out of scope for this PR: - `.agents/skills/fs-cook` - `TRACKER.md` - `docs/readme/fs-cook*.md` - worker-template integration - wrapper skill work (`fs-review`, `fs-fixbug`) - Codex-vs-Claude recipe delegation work ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: - none ## **Manual testing steps** ```gherkin Feature: perps modify-action testability and mobile agentic validation contract Scenario: stable modify action selectors are available on the live BTC market details screen Given mm-4 is running MetaMask Mobile on PerpsMarketDetails for BTC When the user presses the modify button for the open position Then the modify action sheet selector appears And the stable flip option selector appears And the old undefined-derived selector is no longer needed Scenario: mobile agentic validation rejects invalid wait timing fields Given a recipe uses wait_for with timeout instead of timeout_ms When validate-flow-schema.js runs Then the recipe fails with an explicit schema error Scenario: screenshots are copied into the requested artifact directory Given validate-recipe.js runs a recipe with a screenshot step When the recipe passes Then the screenshot file is copied into the passed artifacts directory ``` ## **Screenshots/Recordings** ### **Before** - Live validation had to rely on `undefined-flip_position` - Successful screenshots lived outside the requested artifact root ### **After** - Live selector probe on `mm-4` confirms: - `perps-market-details-modify-button` - `perps-market-details-modify-action-sheet` - `perps-market-details-modify-action-sheet-flip_position` - Successful screenshots copy into the requested artifacts directory ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [ ] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. #### Performance checks (if applicable) - [ ] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [ ] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 2e4dab1 commit 1f92780

10 files changed

Lines changed: 76 additions & 5 deletions

File tree

app/components/UI/Perps/Perps.testIds.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -700,6 +700,7 @@ export const PerpsOrderBookViewSelectorsIDs = {
700700
LONG_BUTTON: 'perps-order-book-long-button',
701701
SHORT_BUTTON: 'perps-order-book-short-button',
702702
MODIFY_BUTTON: 'perps-order-book-modify-button',
703+
MODIFY_ACTION_SHEET: 'perps-order-book-modify-action-sheet',
703704
CLOSE_BUTTON: 'perps-order-book-close-button',
704705
DEPTH_BAND_BUTTON: 'perps-order-book-depth-band-button',
705706
DEPTH_BAND_OPTION: 'perps-order-book-depth-band-option',
@@ -762,6 +763,17 @@ export const PerpsTransactionsViewSelectorsIDs = {
762763
FUNDING_LOAD_MORE_SPINNER: 'perps-transactions-funding-load-more-spinner',
763764
} as const;
764765

766+
// ========================================
767+
// PERPS MODIFY ACTION SHEET SELECTORS
768+
// ========================================
769+
770+
export const PerpsModifyActionSheetSelectorsIDs = {
771+
SHEET: 'perps-modify-action-sheet',
772+
ADD_TO_POSITION: 'perps-modify-action-sheet-add_to_position',
773+
REDUCE_POSITION: 'perps-modify-action-sheet-reduce_position',
774+
FLIP_POSITION: 'perps-modify-action-sheet-flip_position',
775+
} as const;
776+
765777
// ========================================
766778
// PERPS FLIP POSITION CONFIRM SHEET SELECTORS
767779
// ========================================

app/components/UI/Perps/Views/PerpsMarketDetailsView/PerpsMarketDetailsView.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1640,6 +1640,7 @@ const PerpsMarketDetailsView: React.FC<PerpsMarketDetailsViewProps> = () => {
16401640
position={existingPosition ?? undefined}
16411641
onClose={closeModifySheet}
16421642
onReversePosition={handleReversePosition}
1643+
testID={PerpsMarketDetailsViewSelectorsIDs.MODIFY_ACTION_SHEET}
16431644
/>
16441645
)}
16451646

app/components/UI/Perps/Views/PerpsOrderBookView/PerpsOrderBookView.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -818,6 +818,7 @@ const PerpsOrderBookView: React.FC<PerpsOrderBookViewProps> = ({
818818
position={existingPosition ?? undefined}
819819
onClose={closeModifySheet}
820820
onReversePosition={handleReversePosition}
821+
testID={PerpsOrderBookViewSelectorsIDs.MODIFY_ACTION_SHEET}
821822
/>
822823
)}
823824

app/components/UI/Perps/Views/PerpsSelectModifyActionView/PerpsSelectModifyActionView.test.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,17 +38,19 @@ jest.mock(
3838
onClose,
3939
onActionSelect,
4040
position,
41+
testID,
4142
}: {
4243
onClose: () => void;
4344
onActionSelect: (action: string) => void;
4445
position?: Position;
46+
testID?: string;
4547
}) {
4648
const ReactModule = jest.requireActual('react');
4749
const { View, Text, TouchableOpacity } =
4850
jest.requireActual('react-native');
4951
return ReactModule.createElement(
5052
View,
51-
{ testID: 'modify-action-sheet' },
53+
{ testID: testID || 'modify-action-sheet' },
5254
ReactModule.createElement(Text, null, 'Modify Position'),
5355
position &&
5456
ReactModule.createElement(
@@ -126,6 +128,12 @@ describe('PerpsSelectModifyActionView', () => {
126128
expect(screen.getByTestId('modify-action-sheet')).toBeOnTheScreen();
127129
});
128130

131+
it('passes testID through to the modify action sheet', () => {
132+
render(<PerpsSelectModifyActionView testID="custom-modify-sheet" />);
133+
134+
expect(screen.getByTestId('custom-modify-sheet')).toBeOnTheScreen();
135+
});
136+
129137
it('renders add to position option', () => {
130138
render(<PerpsSelectModifyActionView />);
131139

app/components/UI/Perps/Views/PerpsSelectModifyActionView/PerpsSelectModifyActionView.tsx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ interface PerpsSelectModifyActionViewProps {
2323
position?: Position;
2424
onClose?: () => void;
2525
onReversePosition?: (position: Position) => void;
26+
testID?: string;
2627
}
2728

2829
const PerpsSelectModifyActionView: React.FC<
@@ -32,6 +33,7 @@ const PerpsSelectModifyActionView: React.FC<
3233
position: positionProp,
3334
onClose: onExternalClose,
3435
onReversePosition,
36+
testID,
3537
}) => {
3638
const navigation = useNavigation();
3739
const route =
@@ -159,6 +161,7 @@ const PerpsSelectModifyActionView: React.FC<
159161
position={position}
160162
onActionSelect={handleActionSelect}
161163
sheetRef={sheetRef}
164+
testID={testID}
162165
/>
163166
);
164167
};

app/components/UI/Perps/components/PerpsModifyActionSheet/PerpsModifyActionSheet.test.tsx

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import React from 'react';
22
import { render, screen, fireEvent } from '@testing-library/react-native';
33
import PerpsModifyActionSheet from './PerpsModifyActionSheet';
44
import { type Position } from '@metamask/perps-controller';
5+
import { PerpsModifyActionSheetSelectorsIDs } from '../../Perps.testIds';
56
const { mockTheme } = jest.requireActual('../../../../../util/theme');
67

78
// Mock dependencies
@@ -268,6 +269,23 @@ describe('PerpsModifyActionSheet', () => {
268269
expect(screen.getByTestId('test-modify-sheet')).toBeOnTheScreen();
269270
});
270271

272+
it('uses stable default testIDs when none are provided', () => {
273+
render(
274+
<PerpsModifyActionSheet
275+
onClose={mockOnClose}
276+
onActionSelect={mockOnActionSelect}
277+
position={mockPosition}
278+
/>,
279+
);
280+
281+
expect(
282+
screen.getByTestId(PerpsModifyActionSheetSelectorsIDs.SHEET),
283+
).toBeOnTheScreen();
284+
expect(
285+
screen.getByTestId(PerpsModifyActionSheetSelectorsIDs.FLIP_POSITION),
286+
).toBeOnTheScreen();
287+
});
288+
271289
it('renders action items with correct testIDs', () => {
272290
render(
273291
<PerpsModifyActionSheet

app/components/UI/Perps/components/PerpsModifyActionSheet/PerpsModifyActionSheet.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { strings } from '../../../../../../locales/i18n';
1818
import styleSheet from './PerpsModifyActionSheet.styles';
1919
import type { ModifyAction } from './PerpsModifyActionSheet.types';
2020
import { type Position } from '@metamask/perps-controller';
21+
import { PerpsModifyActionSheetSelectorsIDs } from '../../Perps.testIds';
2122

2223
interface ActionOption {
2324
action: ModifyAction;
@@ -41,7 +42,7 @@ const PerpsModifyActionSheet: React.FC<PerpsModifyActionSheetProps> = ({
4142
position,
4243
onActionSelect,
4344
sheetRef: externalSheetRef,
44-
testID,
45+
testID = PerpsModifyActionSheetSelectorsIDs.SHEET,
4546
}) => {
4647
const { styles } = useStyles(styleSheet, {});
4748
const internalSheetRef = useRef<BottomSheetRef>(null);

scripts/perps/agentic/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ Executable gate checks that must pass before a flow runs. Defined in `teams/<tea
231231
| `eval_ref` | `ref`, `assert` | Run a named eval ref |
232232
| `call` | `ref` | Call another flow (workflow nodes only) |
233233
| `wait` | | Pause N ms |
234-
| `wait_for` | condition | Poll until condition met (route/test_id/expression) |
234+
| `wait_for` | condition | Poll until condition met (route/test_id/expression). Canonical timing fields are `timeout_ms` and `poll_ms`. |
235235
| `log_watch` | `watch_for` or `must_not_appear` | Scan Metro logs |
236236
| `screenshot` | | Capture screen |
237237
| `manual` | | Human intervention point |

scripts/perps/agentic/validate-flow-schema.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ function validateActionShape(node, issues) {
8383
if (!node.route && !node.not_route && !node.test_id && !node.expression) {
8484
issues.push(` [${node.id || '?'}] action="wait_for" requires a condition`);
8585
}
86+
if ('timeout' in node && !('timeout_ms' in node)) {
87+
issues.push(
88+
` [${node.id || '?'}] action="wait_for" uses unsupported field "timeout"; use "timeout_ms" instead`
89+
);
90+
}
8691
break;
8792
case 'switch':
8893
if (!Array.isArray(node.cases) || node.cases.length === 0) {

scripts/perps/agentic/validate-recipe.js

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,23 @@ const {
3030

3131
const DEFAULT_LOG_LINES = 400;
3232

33+
function validateSchemaOrThrow(appRoot, recipePath) {
34+
const validatorPath = path.join(__dirname, 'validate-flow-schema.js');
35+
const result = spawnSync('node', [validatorPath, recipePath], {
36+
cwd: appRoot,
37+
encoding: 'utf8',
38+
});
39+
40+
if (result.status === 0) {
41+
return;
42+
}
43+
44+
const output = [result.stdout || '', result.stderr || '']
45+
.join('\n')
46+
.trim();
47+
throw new Error(`Schema validation failed for ${recipePath}\n${output}`);
48+
}
49+
3350
function timestampSlug() {
3451
return new Date()
3552
.toISOString()
@@ -537,11 +554,14 @@ function ensureRunArtifacts(runOptions, recipePath) {
537554
return state.artifacts;
538555
}
539556

557+
const explicitArtifactsDir = runOptions.artifactsDir
558+
? path.resolve(runOptions.artifactsDir)
559+
: '';
540560
const baseDir =
541-
runOptions.artifactsDir ||
561+
explicitArtifactsDir ||
542562
path.join(runOptions.appRoot, '.agent', 'recipe-runs');
543563
const recipeLabel = sanitizeFileSegment(path.basename(recipePath, path.extname(recipePath)));
544-
const rootDir = path.resolve(path.join(baseDir, `${timestampSlug()}_${recipeLabel}`));
564+
const rootDir = explicitArtifactsDir || path.resolve(path.join(baseDir, `${timestampSlug()}_${recipeLabel}`));
545565

546566
const artifacts = {
547567
rootDir,
@@ -1644,6 +1664,8 @@ async function main() {
16441664
throw new Error(`No recipe teams directory found: ${teamsDir}`);
16451665
}
16461666

1667+
validateSchemaOrThrow(appRoot, recipeInput.recipePath);
1668+
16471669
const runOptions = {
16481670
appRoot,
16491671
artifactsDir: options.artifactsDir,

0 commit comments

Comments
 (0)