Skip to content

Commit 514d522

Browse files
committed
Address Copilot review feedback
- Add explicit validation for port 0 (invalid) - Enforce directory-only input for --source option - Use warnings parameter in _check_source_files for clarity - Add test coverage for port 0 and port > 65535 edge cases - Reorder port validation to check range before conflicts
1 parent 6c6d2ae commit 514d522

3 files changed

Lines changed: 61 additions & 3 deletions

File tree

concore_cli/cli.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,12 @@ def run(workflow_file, source, output, type, auto_build):
4747

4848
@cli.command()
4949
@click.argument('workflow_file', type=click.Path(exists=True))
50-
@click.option('--source', '-s', type=click.Path(exists=True), help='Source directory to check file references')
50+
@click.option(
51+
'--source',
52+
'-s',
53+
type=click.Path(exists=True, file_okay=False, dir_okay=True, path_type=Path),
54+
help='Source directory to check file references',
55+
)
5156
def validate(workflow_file, source):
5257
"""Validate a workflow file"""
5358
try:

concore_cli/commands/validate.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,12 @@ def _check_source_files(soup, source_path, errors, warnings):
161161

162162
label = label_tag.text.strip()
163163
if ':' not in label:
164+
warnings.append(f"Skipping node with invalid label format (expected 'ID:filename')")
164165
continue
165166

166167
parts = label.split(':')
167168
if len(parts) != 2:
169+
warnings.append(f"Skipping node '{label}' with invalid format")
168170
continue
169171

170172
_, filename = parts
@@ -230,6 +232,13 @@ def _check_zmq_ports(soup, errors, warnings):
230232
port_name = match.group(2)
231233
port_num = int(port_hex, 16)
232234

235+
if port_num < 1:
236+
errors.append(f"Invalid port number: {port_num} (0x{port_hex}) must be at least 1")
237+
continue
238+
elif port_num > 65535:
239+
errors.append(f"Invalid port number: {port_num} (0x{port_hex}) exceeds maximum (65535)")
240+
continue
241+
233242
if port_num in ports_used:
234243
existing_name = ports_used[port_num]
235244
if existing_name != port_name:
@@ -239,8 +248,6 @@ def _check_zmq_ports(soup, errors, warnings):
239248

240249
if port_num < 1024:
241250
warnings.append(f"Port {port_num} (0x{port_hex}) is in reserved range (< 1024)")
242-
elif port_num > 65535:
243-
errors.append(f"Invalid port number: {port_num} (0x{port_hex}) exceeds maximum (65535)")
244251

245252
def show_results(console, errors, warnings, info):
246253
if errors:

tests/test_graph.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,52 @@ def test_validate_cycle_detection(self):
241241

242242
self.assertIn('cycles', result.output)
243243
self.assertIn('control loops', result.output)
244+
245+
def test_validate_port_zero(self):
246+
content = '''
247+
<graphml xmlns:y="http://www.yworks.com/xml/graphml">
248+
<graph id="G" edgedefault="directed">
249+
<node id="n0">
250+
<data key="d0"><y:NodeLabel>n0:script1.py</y:NodeLabel></data>
251+
</node>
252+
<node id="n1">
253+
<data key="d0"><y:NodeLabel>n1:script2.py</y:NodeLabel></data>
254+
</node>
255+
<edge source="n0" target="n1">
256+
<data key="d1"><y:EdgeLabel>0x0_invalid</y:EdgeLabel></data>
257+
</edge>
258+
</graph>
259+
</graphml>
260+
'''
261+
filepath = self.create_graph_file('port_zero.graphml', content)
262+
263+
result = self.runner.invoke(cli, ['validate', filepath])
264+
265+
self.assertIn('Validation failed', result.output)
266+
self.assertIn('must be at least 1', result.output)
267+
268+
def test_validate_port_exceeds_maximum(self):
269+
content = '''
270+
<graphml xmlns:y="http://www.yworks.com/xml/graphml">
271+
<graph id="G" edgedefault="directed">
272+
<node id="n0">
273+
<data key="d0"><y:NodeLabel>n0:script1.py</y:NodeLabel></data>
274+
</node>
275+
<node id="n1">
276+
<data key="d0"><y:NodeLabel>n1:script2.py</y:NodeLabel></data>
277+
</node>
278+
<edge source="n0" target="n1">
279+
<data key="d1"><y:EdgeLabel>0x10000_toobig</y:EdgeLabel></data>
280+
</edge>
281+
</graph>
282+
</graphml>
283+
'''
284+
filepath = self.create_graph_file('port_max.graphml', content)
285+
286+
result = self.runner.invoke(cli, ['validate', filepath])
287+
288+
self.assertIn('Validation failed', result.output)
289+
self.assertIn('exceeds maximum (65535)', result.output)
244290

245291
if __name__ == '__main__':
246292
unittest.main()

0 commit comments

Comments
 (0)