fix: address second round of code-review findings

actions.js:
- Add assertValidRepo/assertValidIssueNumber guards at the top of all
  action handlers (applyClaim, applySync, applyValidate, applyPublish,
  applyReview, applyDecompose, applyUnblock) for fast-fail validation
- applyValidate: fix status transition — set 'validated' unconditionally
  when ok=true instead of preserving 'blocked' (was inconsistent with
  projectState becoming 'ready')

gh-api.js:
- runGh: preserve GITHUB_TOKEN by default; only delete when caller
  explicitly sets options.stripGithubToken=true (was deleting by
  default, breaking CI)

parsing.js:
- extractCoordinationState: throw SyntaxError on malformed JSON instead
  of silently returning null — lets callers distinguish bad JSON from
  absent marker
- normalizeBodyForComparison: fix regex to match JSON-quoted form
  "lastSyncAt": ... instead of bare lastSyncAt: ...

policy.js:
- loadPolicy: validate that parsed JSON is a plain object before
  spreading; coerce nested fields (labels, review, validation,
  branchModel, project, fieldNames) to objects before merging

state.js:
- assertIssueClaimable: block re-claim on status alone (not status AND
  owner) to prevent {status:'claimed', owner:null} bypass; use
  state.owner || 'unknown' in error message
- getCoordinationState: catch SyntaxError from extractCoordinationState,
  log warning to stderr, fall back to default state

tests/lib:
- Update malformed-JSON test to expect SyntaxError throw instead of null

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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 });

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;
   }
 

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) {
+  } catch (error) {
-    return null;
+    throw new SyntaxError(
+      `Malformed coordination JSON in body: ${error.message} — raw: ${match[1].slice(0, 120)}`
+    );
   }
 }
 

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: {
+    labels: { ...DEFAULT_LABELS, ...labels },
-      ...DEFAULT_LABELS,
+    review: { ...DEFAULT_POLICY.review, ...review },
-      ...(parsed.labels || {}),
+    validation: { ...DEFAULT_POLICY.validation, ...validation },
-    },
+    branchModel: { ...DEFAULT_POLICY.branchModel, ...branchModel },
-    review: {
-      ...DEFAULT_POLICY.review,
-      ...(parsed.review || {}),
-    },
-    validation: {
-      ...DEFAULT_POLICY.validation,
-      ...(parsed.validation || {}),
-    },
-    branchModel: {
-      ...DEFAULT_POLICY.branchModel,
-      ...(parsed.branchModel || {}),
-    },
     project: {
       ...DEFAULT_POLICY.project,
-      ...(parsed.project || {}),
+      ...project,
-      fieldNames: {
+      fieldNames: { ...DEFAULT_POLICY.project.fieldNames, ...fieldNames },
-        ...DEFAULT_POLICY.project.fieldNames,
-        ...((parsed.project || {}).fieldNames || {}),
-      },
     },
     sourcePath: resolvedPath,
   };

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) {
+  if (state.status === 'claimed') {
-    throw new Error(`Issue #${issue.number} is already claimed by ${state.owner}`);
+    throw new Error(`Issue #${issue.number} is already claimed by ${state.owner || 'unknown'}`);
   }
 }
 

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 = [
         '<!-- ecc-coordination:start -->',
@@ -109,8 +109,7 @@ const DESCRIPTORS = [
         '```',
         '<!-- ecc-coordination:end -->',
       ].join('\n');
-      const result = extractCoordinationState(body);
+      assert.throws(() => extractCoordinationState(body), SyntaxError);
-      assert.strictEqual(result, null);
     },
   },