Skip to content

Commit 331b1a4

Browse files
fix: address high priority code review issues
- Fix secrets parsing to properly extract required field from inline metadata - Add context to installWorkflow error messages - Add test for required: false secrets parsing - Update test fixture with mixed required values - Remove testing branch from CI triggers
1 parent a75f281 commit 331b1a4

5 files changed

Lines changed: 19 additions & 7 deletions

File tree

.github/workflows/test.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ name: Tests
22

33
on:
44
push:
5-
branches: [main, master, testing]
5+
branches: [main, master]
66
pull_request:
7-
branches: [main, master, testing]
7+
branches: [main, master]
88

99
jobs:
1010
test:

src/core/registry.test.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,14 +95,20 @@ describe('WorkflowRegistry', () => {
9595
expect(workflow?.type).toBe('set');
9696
});
9797

98-
test('should parse multiple secrets', async () => {
98+
test('should parse multiple secrets with required field', async () => {
9999
await registry.load();
100100
const workflow = registry.getWorkflow('test-category/another-workflow');
101101

102102
expect(workflow?.metadata.secrets.length).toBe(2);
103103
const secretNames = workflow?.metadata.secrets.map(s => s.name);
104104
expect(secretNames).toContain('API_KEY');
105105
expect(secretNames).toContain('TOKEN');
106+
107+
// Verify required field is parsed correctly
108+
const apiKeySecret = workflow?.metadata.secrets.find(s => s.name === 'API_KEY');
109+
const tokenSecret = workflow?.metadata.secrets.find(s => s.name === 'TOKEN');
110+
expect(apiKeySecret?.required).toBe(true);
111+
expect(tokenSecret?.required).toBe(false);
106112
});
107113
});
108114

src/core/registry.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -273,12 +273,15 @@ export class WorkflowRegistry {
273273
const secrets: SecretRequirement[] = [];
274274
const secretsBlock = content.match(/# secrets:[\s\S]*?(?=\n# [a-z]|\n\n|$)/i);
275275
if (secretsBlock) {
276-
const regex = /#\s+- name:\s*(\S+)[\s\S]*?#\s+description:\s*(.+)/g;
277-
for (const item of secretsBlock[0].matchAll(regex)) {
276+
// Match individual secret entries with name, description, and optional required field
277+
const entryRegex = /#\s+- name:\s*(\S+)[\s\S]*?#\s+description:\s*([^\n]+)(?:[\s\S]*?#\s+required:\s*(true|false))?/g;
278+
for (const item of secretsBlock[0].matchAll(entryRegex)) {
278279
const name = item[1];
279280
const description = item[2];
281+
const requiredStr = item[3];
280282
if (!name || !description) continue;
281-
secrets.push({ name, description: description.trim(), required: true });
283+
const required = requiredStr ? requiredStr.trim() === 'true' : true;
284+
secrets.push({ name, description: description.trim(), required });
282285
}
283286
}
284287

src/test/fixtures/test-category/another-workflow/another-workflow.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88
# secrets:
99
# - name: API_KEY
1010
# description: API key for service
11+
# required: true
1112
# - name: TOKEN
1213
# description: Authentication token
14+
# required: false
1315
# triggers:
1416
# - schedule
1517
# variants:

src/tui/utils/install-workflow.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,10 @@ export async function installWorkflow(
8282
},
8383
};
8484
} catch (error) {
85+
const message = error instanceof Error ? error.message : 'Unknown error occurred';
8586
return {
8687
success: false,
87-
message: error instanceof Error ? error.message : 'Unknown error occurred',
88+
message: `Failed to install workflow: ${message}`,
8889
};
8990
}
9091
}

0 commit comments

Comments
 (0)