Skip to content

Fail closed on effective-rules rows with a missing or non-string type #141

Closed

Description

@unbraind

Contract and scope

At 247b95e6214f687d0d838d1685cc0e99db664cd6, the closeout documentation promises that missing, malformed, or incomplete evidence fails closed. The readable-empty fallback is allowed only when both policy sources establish that no required contexts are configured.

This is a reproducible malformed-response validation gap. It is not a claim that GitHub currently emits the malformed responses below, nor an observed production incident. GitHub's effective branch rules endpoint returns typed rule objects; a row without a string discriminator is not a valid unrelated rule.

Reproduction

Use an isolated real SDK tracker containing an in_progress Task, a merged PR linking that Task, and synthetic HTTP responses through the existing fetchJSON/parseNextLink transport:

  1. Classic required-status-check metadata returns HTTP 200 with { "contexts": [], "checks": [] }
  2. Effective branch rules return HTTP 200 with:
[
  {
    "parameters": {
      "required_status_checks": [
        { "context": "not-run", "integration_id": 7 }
      ]
    }
  }
]
  1. The exact PR head has one completed successful check named optional-CI; no not-run check or status exists. Legacy statuses return []
  2. Call runCloseout(root, "fixture/project", { pr: "42", apply: true }, api)

Observed: closed=1, failed=0; the real Task changes to closed. The report says rule="fallback", code="head_checks_passed", and asserts that protection metadata is readable with no required contexts.

Expected: refuse closure with unavailable/incomplete evidence and leave durable tracker content unchanged. A malformed policy row cannot establish an empty required-check policy.

Controls and extent

  • Add only "type": "required_status_checks" to the same row: closed=0; the Task remains in_progress, and durable tracker files remain byte-identical
  • Replace the row with the valid unrelated rule { "type": "deletion" }: closed=1 under the documented fallback
  • An independent 22-case head-policy matrix passes 18 cases but also reproduces unsafe acceptance for [{}], [{"type":null}], and [{"type":4}]
  • HTTP 403/404 without declarations, unsuccessful/missing declarations, readable app bindings, latest legacy statuses, malformed page shapes, and valid unrelated rules behave as documented in that matrix

Root cause: closeout.ts lines 180–182 only validates that each row is an object. Every discriminator unequal to required_status_checks, including absent and non-string values, is silently skipped. Validate the discriminator before treating a row as an unrelated rule.

Suggested regression coverage: missing/null/non-string rule types should refuse with zero SDK closes and no durable writes, while a well-formed unrelated rule remains accepted. Include the declared-policy path so declarations cannot disguise malformed metadata as a verified source.

Duplicate check

Reviewed canonical tracker records and all-status GitHub collections on 2026-10-16: 140 issue/PR entries and 136 PRs across two pages each. No matching malformed-discriminator issue was found. #138/#140 address unavailable HTTP 403/404 metadata; this case receives HTTP 200 with structurally invalid rows and is not covered by the existing malformed-rules tests.

The recorded reproduction used disposable fixtures with PM telemetry explicitly disabled. No live trackers, repository settings, workflow pins, or GitHub integrations were changed.

Activity

  1. unbraind commented on Oct 18, 2026

    @unbraind
    OwnerAuthor

    Fixed in PR #148 at f94e4e5e7f06b5a3d443f082a1b4f4215c967534 (pushed). Effective rule types must be nonempty strings before they can count as unrelated metadata. node --test --test-name-pattern="malformed rule discriminator|valid unrelated effective rule" test/closeout.test.ts passes 13/13 real-SDK cases: 12 zero-close, byte-identical durable-state refusals across readable-empty/declared paths and the valid unrelated-rule fallback control. Reverting only closeout.ts fails the isolated missing-type regression; the original-code matrix fails all 12 malformed cases. Full isolated npm/Bun gates and CI are in progress.

  2. unbraind commented on Oct 18, 2026

    @unbraind
    OwnerAuthor

    Final delivery for PR #148 is pushed at 96a4b4bedea2943797a02510e7f4e7aad395b8f6. Real-SDK malformed-policy/control matrix: 13/13 pass; isolated fix revert fails, and all 12 malformed cases preserve durable tracker bytes with the fix. Both isolated CI release commands pass 824/824 with zero failures/skips and exact 100% in all four coverage dimensions across 14 modules. Final-head Node 22/26, native Bun acceptance, CodeQL and Semgrep pass: https://github.com/unbraind/pm-github/actions/runs/39098874412 . Implementation and tests match the release-gated source; the temporary validation workflow was removed. No PR was merged.

  3. unbraind commented on Oct 18, 2026

    @unbraind
    OwnerAuthor

    Revalidated after integrating current main in pushed head 04bbf3603c30e6dd81efa2622e3aa0d086f845eb. node --test --test-name-pattern='malformed rule discriminator|valid unrelated effective rule' test/closeout.test.ts passes 13/13. Both exact isolated-identity npm run release:check and bun run release:check pass 836/836 with exact 100/100/100/100 coverage across all 14 authored modules: https://github.com/unbraind/pm-github/actions/runs/39105101037. The validated head was 762285d94e90e645927b90e8032292c191772b49; final production code, tests, scripts, docs and dependency pins are byte-identical. Existing behavioral revert proof is preserved, and PR #148 retains its closing link. Final-head CI is tracked in the PR body; this comment does not close the issue.

  4. unbraind commented on Oct 18, 2026

    @unbraind
    OwnerAuthor

    PR #148 is pushed at 7d4306774005b5542ab617926593252c6b319f80. node --test --test-name-pattern='malformed rule discriminator|valid unrelated effective rule' test/closeout.test.ts passes 13/13. Original malformed-policy fix and recorded production-only revert proof are retained after merging current main.

    Both exact isolated npm run release:check and bun run release:check pass 893/893 with zero failures/skips/cancellations/todos and exact 100/100/100/100 source coverage: https://github.com/unbraind/pm-github/actions/runs/39119457879 . Fresh-clone strict health passes. Final-head Node 22/26 CI, including native Bun acceptance, passes (https://github.com/unbraind/pm-github/actions/runs/39120403305); CodeQL and Semgrep pass; no merge or release was performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions