IssueSyncer #formatIssueMarkdown null-safety + comprehensive unit-test sweep (#11474 follow-up)

Context

PR #11476 (resolving #11474) added defensive null-safety to IssueSyncer.#formatTimelineEvent but did NOT sweep the same null-deref pattern in #formatIssueMarkdown (the frontmatter-assembly method that runs PER ISSUE). Operator re-ran npm run ai:sync-github-workflow post-merge and hit the same error class at a different line:

❌ Sync failed: TypeError: Cannot read properties of null (reading 'login')
    at #formatIssueMarkdown (ai/services/github-workflow/sync/IssueSyncer.mjs:145:46)
    at IssueSyncer.pullFromGitHub (IssueSyncer.mjs:594:59)

Line 145: issue.author.login — issue with deleted GitHub author (Ghost user).

This is the whack-a-mole anti-pattern that #11474's own "Avoided Traps" section explicitly warned against:

Only fixing AssignedEvent (the empirical crash): rejected — whack-a-mole; next sync hits a null label / subIssue etc. Defensive coverage across the whole switch is the right scope.

I extended that discipline to the timeline-events switch but did NOT extend it ACROSS the rest of the file. Same root pattern, different surface. Operator surfaced the gap empirically + emphasized: "we need unit testing to ensure it works."

Problem

IssueSyncer.#formatIssueMarkdown has FIVE additional null-deref sites (in addition to the timeline-event ones already fixed):

Sibling syncers already handle this correctly (V-B-A 2026-05-16):

• PullRequestSyncer.mjs:250: author: pr.author?.login || 'unknown' ✓
• DiscussionSyncer.mjs:244: author: discussion.author?.login || 'unknown' ✓
• DiscussionSyncer.mjs:256: comment.author?.login || 'unknown' ✓

Only IssueSyncer.#formatIssueMarkdown is incomplete. The earlier #formatTimelineEvent fix from PR #11476 was scope-correct for THAT method but should have been a same-PR sweep across the file.

Architectural Reality

#formatIssueMarkdown assembles YAML frontmatter for every issue's markdown file. Runs once per issue during sync. ONE null entity in ANY issue's frontmatter aborts the whole sync run. With 8,500+ issues processed, the probability of hitting a Ghost-user issue is high.

The fix shape established in PR #11476 + present in sibling syncers is: optional chaining + fallback markers. Apply uniformly.

The operator-emphasized testing dimension: unit tests must exercise the full failure surface end-to-end, not just one method. The PR #11476 test covered #formatTimelineEvent with null events; it should have ALSO covered #formatIssueMarkdown with null frontmatter fields. Same test surface (refetchIssuesByNumber pipeline), broader mock coverage.

The Fix

Two-part:

Part 1: Comprehensive null-safety sweep in #formatIssueMarkdown

const frontmatter = {
    id           : issue.number,
    title        : issue.title?.replace(lineBreaksRegex, ' ') || '(no title)',
    state        : issue.state,
    labels       : (issue.labels?.nodes || []).map(l => l?.name).filter(Boolean),
    assignees    : (issue.assignees?.nodes || []).map(a => a?.login).filter(Boolean),
    createdAt    : issue.createdAt,
    updatedAt    : issue.updatedAt,
    githubUrl    : issue.url,
    author       : issue.author?.login || 'Ghost',
    commentsCount: this.#countTimelineComments(issue),
    parentIssue  : issue.parent?.number ?? null,
    subIssues    : (issue.subIssues?.nodes || []).map(sub =>
        sub ? `[${sub.state === 'CLOSED' ? 'x' : ' '}] ${sub.number} ${sub.title?.replace(lineBreaksRegex, ' ') || '(no title)'}` : null
    ).filter(Boolean),
    subIssuesCompleted: issue.subIssuesSummary?.completed || 0,
    subIssuesTotal    : issue.subIssuesSummary?.total || 0,
    blockedBy: (issue.blockedBy?.nodes || []).map(b =>
        b ? `[${b.state === 'CLOSED' ? 'x' : ' '}] ${b.number} ${b.title?.replace(lineBreaksRegex, ' ') || '(no title)'}` : null
    ).filter(Boolean),
    blocking: (issue.blocking?.nodes || []).map(b =>
        b ? `[${b.state === 'CLOSED' ? 'x' : ' '}] ${b.number} ${b.title?.replace(lineBreaksRegex, ' ') || '(no title)'}` : null
    ).filter(Boolean)
};

// ...
let body = `# ${issue.title || '(no title)'}\n\n`;

Fallback marker conventions match PR #11476 (Ghost for users; filter out null entries from list-mapped fields; '(no title)' for missing titles).

Part 2: Comprehensive unit-test sweep

Add tests to test/playwright/unit/ai/services/github-workflow/IssueSyncer.spec.mjs covering:

• issue.author = null (the empirical crash; Ghost user)
• issue.labels.nodes containing null entries / null label names
• issue.assignees.nodes containing null entries / null login
• issue.subIssues.nodes containing null entries / null sub.title
• issue.blockedBy.nodes containing null entries
• Combined "ghost issue" test: ALL nullable fields null simultaneously

Each test drives through IssueSyncer.refetchIssuesByNumber (the existing test pipeline) and asserts:

• No crash
• Rendered markdown contains expected fallback markers
• No literal undefined/null artifacts leaked into output

Acceptance Criteria

• All null-deref sites in IssueSyncer.#formatIssueMarkdown use optional chaining + fallback markers (per the Fix section above).
• Fallback markers consistent with PR #11476's convention: 'Ghost' for users, '(no title)' for missing titles, filter-out for null list entries.
• New unit tests cover author/labels/assignees/subIssues/blockedBy null cases; plus one combined "ghost issue" test with all fields null.
• Existing 6/6 IssueSyncer specs continue to pass (no regression).
• Same audit applied to #formatIssueMarkdown siblings if any exist (cursory grep confirms none in IssueSyncer; PullRequestSyncer + DiscussionSyncer already correct per Architectural Reality section).
• Operator empirical verification: post-merge npm run ai:sync-github-workflow processes past the v8.1.0 chunk-28 crash point AND any subsequent null-entity issue in the corpus.

Out of Scope

• Refactoring #formatIssueMarkdown into a more declarative renderer (out-of-scope cleanup).
• Adding GraphQL-side null-filter on the FETCH_ISSUES_FOR_SYNC query (returns less data, loses archive context).
• Sweeping null-safety across non-syncer code paths (the syncers are the empirical pain surface).
• Wider regression-test fixture for the FULL sync pipeline (would require GraphQL replay infrastructure; substrate-shaped follow-up if friction recurs).

Avoided Traps

• Whack-a-mole pattern (the trap THIS ticket exists to break): rejected — must sweep ALL null-deref sites in the method, not just the empirical crash line. Operator explicitly emphasized "we need unit testing to ensure it works" — comprehensive test coverage is the safety net against future whack-a-mole.
• Filtering null events at fetch time: rejected (same as #11474) — loses archive context.
• Adding nullable-field-checks via Zod schema in services.mjs: rejected — runtime validation at SDK boundary is heavyweight for what's a render-time defensive concern. Optional-chaining at the render site is the right scope.

Related

• Direct parent: #11474 / PR #11476 (null-safety on #formatTimelineEvent only — this ticket extends to #formatIssueMarkdown)
• Sibling correctness references: PullRequestSyncer.mjs:250 + DiscussionSyncer.mjs:244+256 (already use the right pattern)
• Empirical anchor: operator's 2026-05-16 sync_all run from main checkout post-#11476-merge, crashed at IssueSyncer.mjs:145 after processing issues through v8.1.0/chunk-28
• Operator framing 2026-05-16: "we need unit testing to ensure it works"

Origin Session ID: 0064efde-455e-4ecd-a26f-574381b3766a

Retrieval Hint: query_raw_memories(query="IssueSyncer formatIssueMarkdown null author Ghost frontmatter assembly null-safety follow-up whack-a-mole #11474")