Skip to content

Code review 2026-05-02 — 49c60da #189

@Luis85

Description

@Luis85

2026-05-02 — 49c60da

Reviewed: 5ce68c3..49c60da (4 commits)
Blockers: 0 | Majors: 0 | Minors: 1 | Nits: 1


  • [MINOR] scripts/create-v1-issues.mjs:91 — POST requests missing Content-Type: application/json; issue creation may fail

    details

    Problem: gh() helper never sets Content-Type: application/json on POST requests that pass a JSON-serialised body

    Why it matters: Node's native fetch does not auto-add Content-Type for plain string bodies; if the GitHub API enforces the header (per HTTP spec), all 16 issue-create calls fail with 415 Unsupported Media Type or the body is silently ignored, leaving no issues created and no actionable error from the validation layer

    Fix:

     async function gh(path, init = {}) {
       const res = await fetch(`https://api.github.com${path}`, {
         ...init,
         headers: {
           Accept: 'application/vnd.github+json',
           Authorization: `Bearer ${token}`,
           'X-GitHub-Api-Version': '2022-11-28',
    +      ...(init.body !== undefined ? { 'Content-Type': 'application/json' } : {}),
           ...(init.headers ?? {}),
         },
       });
  • [NIT] scripts/create-v1-issues.mjs:18GITHUB_REPOSITORY validation accepts multi-segment paths silently

    details

    Problem: repository?.includes('/') only checks for the presence of a slash; owner/suborg/repo passes validation, then const [owner, repo] = repository.split('/') silently drops the third segment and targets the wrong repo

    Why it matters: Issues would be created in owner/suborg instead of owner/suborg/repo; the error message says "expected owner/repo" but the guard does not enforce that invariant

    Fix:

    -if (!repository?.includes('/')) {
    +const parts = repository?.split('/') ?? [];
    +if (parts.length !== 2 || !parts[0] || !parts[1]) {
       console.error('Missing/invalid GITHUB_REPOSITORY (expected owner/repo).');
       process.exit(1);
     }
    -const [owner, repo] = repository.split('/');
    +const [owner, repo] = parts;

  • Reviewed range: 5ce68c3077907fb998385fa428b3656043db2577..49c60da5611be873713af8e2c36819da373fa3f6 (4 commits, 3 files)
  • Blockers: 0
  • Majors: 0
  • Minors: 1
  • Nits: 1
  • Counter-argument check: 49c60da.1 tested — GitHub's API is reportedly lenient with Content-Type for JSON bodies, and the post-merge fix commits fix(scripts): honour ISSUE_LABELS override in create-v1-issues #187/fix(scripts): resolve merge conflicts in create-v1-issues branch #188 didn't address this, suggesting the script ran successfully without it. However, relying on undocumented server leniency is fragile. Finding retained as [MINOR].
  • Not reviewed: lockfiles, generated files, prior daily-review docs (doc-only snapshots, out of scope)
  • Last reviewed SHA: 49c60da5611be873713af8e2c36819da373fa3f6

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions