diff --git a/scripts/lib/github-coordination/actions.js b/scripts/lib/github-coordination/actions.js index 6ad73b5e..2b00b249 100644 --- a/scripts/lib/github-coordination/actions.js +++ b/scripts/lib/github-coordination/actions.js @@ -16,6 +16,18 @@ const { const { upsertCoordinationWorkItem } = require('./store'); const { extractIssueReferences, extractTasks } = require('./parsing'); +function assertValidRepo(repo) { + if (typeof repo !== 'string' || !repo.trim()) { + throw new Error(`invalid repo: expected non-empty string, got ${JSON.stringify(repo)}`); + } +} + +function assertValidIssueNumber(issueNumber) { + if (!Number.isFinite(issueNumber) || issueNumber <= 0 || !Number.isInteger(issueNumber)) { + throw new Error(`invalid issueNumber: expected positive integer, got ${JSON.stringify(issueNumber)}`); + } +} + // applyClaim performs a read (getIssue) → check (assertIssueClaimable) → write // (editIssue) sequence that is NOT atomic. Two concurrent callers can both read // an unclaimed issue, pass the check, and both succeed — resulting in a @@ -24,6 +36,8 @@ const { extractIssueReferences, extractTasks } = require('./parsing'); // branch-protection rule that allows only one actor at a time, or a // serialized job queue). function applyClaim(repo, issueNumber, options = {}, context = {}) { + assertValidRepo(repo); + assertValidIssueNumber(issueNumber); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const store = context.store || null; const issue = getIssue(repo, issueNumber, options); @@ -59,6 +73,7 @@ function applyClaim(repo, issueNumber, options = {}, context = {}) { } function applySync(repo, options = {}, context = {}) { + assertValidRepo(repo); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const store = context.store || null; const issues = listIssues(repo, { ...options, state: options.state || 'all', limit: options.limit || 100 }); @@ -107,6 +122,8 @@ function applySync(repo, options = {}, context = {}) { } function applyValidate(repo, issueNumber, options = {}, context = {}, existingIssue = null) { + assertValidRepo(repo); + assertValidIssueNumber(issueNumber); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const issue = existingIssue || getIssue(repo, issueNumber, options); const state = getCoordinationState(issue, policy); @@ -129,7 +146,7 @@ function applyValidate(repo, issueNumber, options = {}, context = {}, existingIs const ok = validations.every(entry => entry.ok); const nextState = buildIssueStateFromAction(issue, state, 'validate', { - status: ok ? (state.status === 'blocked' ? 'blocked' : 'validated') : state.status, + status: ok ? 'validated' : state.status, validation: ok ? 'passed' : 'failed', projectState: ok ? 'ready' : (state.project && state.project.state) || 'backlog', }, policy); @@ -157,6 +174,8 @@ function applyValidate(repo, issueNumber, options = {}, context = {}, existingIs } function applyPublish(repo, issueNumber, options = {}, context = {}) { + assertValidRepo(repo); + assertValidIssueNumber(issueNumber); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const issue = getIssue(repo, issueNumber, options); const state = getCoordinationState(issue, policy); @@ -194,6 +213,8 @@ function applyPublish(repo, issueNumber, options = {}, context = {}) { } function applyReview(repo, issueNumber, options = {}, context = {}) { + assertValidRepo(repo); + assertValidIssueNumber(issueNumber); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const issue = getIssue(repo, issueNumber, options); const state = getCoordinationState(issue, policy); @@ -225,6 +246,8 @@ function applyReview(repo, issueNumber, options = {}, context = {}) { } function applyDecompose(repo, issueNumber, options = {}, context = {}) { + assertValidRepo(repo); + assertValidIssueNumber(issueNumber); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const issue = getIssue(repo, issueNumber, options); const state = getCoordinationState(issue, policy); @@ -264,6 +287,7 @@ function applyDecompose(repo, issueNumber, options = {}, context = {}) { } function applyUnblock(repo, options = {}, context = {}) { + assertValidRepo(repo); const policy = context.policy || loadPolicy(context.rootDir || process.cwd(), options.configPath); const store = context.store || null; const issues = listIssues(repo, { ...options, state: 'all', limit: options.limit || 100 }); diff --git a/scripts/lib/github-coordination/gh-api.js b/scripts/lib/github-coordination/gh-api.js index 98f15560..d7669cdb 100644 --- a/scripts/lib/github-coordination/gh-api.js +++ b/scripts/lib/github-coordination/gh-api.js @@ -64,7 +64,7 @@ function runGh(args, options = {}) { const commandArgs = shimPath ? [shimPath, ...args] : args; const env = { ...process.env }; - if (!options.useEnvGithubToken) { + if (options.stripGithubToken) { delete env.GITHUB_TOKEN; } diff --git a/scripts/lib/github-coordination/parsing.js b/scripts/lib/github-coordination/parsing.js index 3b148d21..08bc3b54 100644 --- a/scripts/lib/github-coordination/parsing.js +++ b/scripts/lib/github-coordination/parsing.js @@ -7,7 +7,7 @@ function escapeRegExp(str) { } function normalizeBodyForComparison(body) { - return (body || '').replace(/lastSyncAt:\s*[^\n]+/g, 'lastSyncAt: NORMALIZED'); + return (body || '').replace(/"lastSyncAt"\s*:\s*[^,\}\n]+/g, '"lastSyncAt": NORMALIZED'); } function extractCoordinationState(body, policy = DEFAULT_POLICY) { @@ -27,8 +27,10 @@ function extractCoordinationState(body, policy = DEFAULT_POLICY) { try { const parsed = JSON.parse(match[1]); return parsed && typeof parsed === 'object' ? parsed : null; - } catch (_error) { - return null; + } catch (error) { + throw new SyntaxError( + `Malformed coordination JSON in body: ${error.message} — raw: ${match[1].slice(0, 120)}` + ); } } diff --git a/scripts/lib/github-coordination/policy.js b/scripts/lib/github-coordination/policy.js index 72fc7cab..dc0b0d54 100644 --- a/scripts/lib/github-coordination/policy.js +++ b/scripts/lib/github-coordination/policy.js @@ -65,32 +65,26 @@ function loadPolicy(rootDir = process.cwd(), configPath = null) { } catch (error) { throw new Error(`Failed to load policy from ${resolvedPath}: ${error.message}`); } + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error(`Policy file ${resolvedPath} must contain a JSON object, got ${Array.isArray(parsed) ? 'array' : typeof parsed}`); + } + const labels = typeof parsed.labels === 'object' && parsed.labels !== null && !Array.isArray(parsed.labels) ? parsed.labels : {}; + const review = typeof parsed.review === 'object' && parsed.review !== null && !Array.isArray(parsed.review) ? parsed.review : {}; + const validation = typeof parsed.validation === 'object' && parsed.validation !== null && !Array.isArray(parsed.validation) ? parsed.validation : {}; + const branchModel = typeof parsed.branchModel === 'object' && parsed.branchModel !== null && !Array.isArray(parsed.branchModel) ? parsed.branchModel : {}; + const project = typeof parsed.project === 'object' && parsed.project !== null && !Array.isArray(parsed.project) ? parsed.project : {}; + const fieldNames = typeof project.fieldNames === 'object' && project.fieldNames !== null && !Array.isArray(project.fieldNames) ? project.fieldNames : {}; return { ...DEFAULT_POLICY, ...parsed, - labels: { - ...DEFAULT_LABELS, - ...(parsed.labels || {}), - }, - review: { - ...DEFAULT_POLICY.review, - ...(parsed.review || {}), - }, - validation: { - ...DEFAULT_POLICY.validation, - ...(parsed.validation || {}), - }, - branchModel: { - ...DEFAULT_POLICY.branchModel, - ...(parsed.branchModel || {}), - }, + labels: { ...DEFAULT_LABELS, ...labels }, + review: { ...DEFAULT_POLICY.review, ...review }, + validation: { ...DEFAULT_POLICY.validation, ...validation }, + branchModel: { ...DEFAULT_POLICY.branchModel, ...branchModel }, project: { ...DEFAULT_POLICY.project, - ...(parsed.project || {}), - fieldNames: { - ...DEFAULT_POLICY.project.fieldNames, - ...((parsed.project || {}).fieldNames || {}), - }, + ...project, + fieldNames: { ...DEFAULT_POLICY.project.fieldNames, ...fieldNames }, }, sourcePath: resolvedPath, }; diff --git a/scripts/lib/github-coordination/state.js b/scripts/lib/github-coordination/state.js index 04e8c747..3f094dc1 100644 --- a/scripts/lib/github-coordination/state.js +++ b/scripts/lib/github-coordination/state.js @@ -36,7 +36,13 @@ function defaultCoordinationState(issue, policy = DEFAULT_POLICY) { function getCoordinationState(issue, policy = DEFAULT_POLICY) { const { extractCoordinationState } = require('./parsing'); // lazy to avoid circular init order - const existing = extractCoordinationState(issue && issue.body, policy); + let existing; + try { + existing = extractCoordinationState(issue && issue.body, policy); + } catch (error) { + process.stderr.write(`[github-coordination] Warning: ${error.message} (issue #${issue && issue.number})\n`); + existing = null; + } if (existing) { return { ...defaultCoordinationState(issue, policy), @@ -205,8 +211,8 @@ function assertIssueClaimable(issue, state) { throw new Error(`Issue #${issue.number} is not open`); } - if (state.status === 'claimed' && state.owner) { - throw new Error(`Issue #${issue.number} is already claimed by ${state.owner}`); + if (state.status === 'claimed') { + throw new Error(`Issue #${issue.number} is already claimed by ${state.owner || 'unknown'}`); } } diff --git a/tests/lib/github-coordination.test.js b/tests/lib/github-coordination.test.js index c71a4115..a983580f 100644 --- a/tests/lib/github-coordination.test.js +++ b/tests/lib/github-coordination.test.js @@ -100,7 +100,7 @@ const DESCRIPTORS = [ }, { group: 'extractCoordinationState', - name: 'extractCoordinationState returns null when JSON block is malformed', + name: 'extractCoordinationState throws SyntaxError when JSON block is malformed', fn: () => { const body = [ '', @@ -109,8 +109,7 @@ const DESCRIPTORS = [ '```', '', ].join('\n'); - const result = extractCoordinationState(body); - assert.strictEqual(result, null); + assert.throws(() => extractCoordinationState(body), SyntaxError); }, },