Skip to content

chore(privacy): address Fro Bot NBCs on private-wiki gate#3330

Merged
marcusrbrown merged 1 commit into
mainfrom
chore/private-wiki-gate-nbcs
May 20, 2026
Merged

chore(privacy): address Fro Bot NBCs on private-wiki gate#3330
marcusrbrown merged 1 commit into
mainfrom
chore/private-wiki-gate-nbcs

Conversation

@marcusrbrown
Copy link
Copy Markdown
Collaborator

Addresses the four non-blocking concerns and two missing-test items Fro Bot raised on PR #3329 (private-wiki gate v1).

What changed

scripts/check-wiki-private-presence.ts

Parameterize the GraphQL call. node_id is no longer interpolated into the query string. The call now uses gh api graphql -F nodeId=<value> -f query='query($nodeId: ID!) { node(id: $nodeId) { ... } }'. A crafted node_id with " or ) previously produced malformed GraphQL → API reject → script failed closed; the injection surface is gone entirely, and error messages are clearer too. Same pattern survey-repo.yaml already uses.

Distinguish failure modes. resolveCanonicalSlugs internally tracks each unresolved entry as subprocess-threw (network/rate-limit/auth) or node-null (deleted repo, revoked App access). The thrown error labels each entry with its mode and a targeted remediation hint:

check-wiki-private-presence: cannot verify private wiki presence (2 entries unresolved):
  [subprocess-threw] R_kgDOABCDEF (investigate network/rate-limit/auth)
  [node-null] R_kgDOXYZ123 (investigate repo lifecycle/App access)

Operators can act per-entry without grep-and-guess.

Normalize canonical slug at storage. resolveCanonicalSlugs now applies .toLowerCase() before storing the resolved slug. GitHub preserves nameWithOwner case (so bfra-me/Works could come back capitalized); the comparison already used .toLowerCase() on both sides, but storage wasn't normalized. Defensive cleanup — if the comparison ever changes, the storage shape stays correct.

.github/workflows/merge-data.yaml

Added a two-line comment above the second checkout explaining the dual-checkout rationale: scripts live on main, the wiki/metadata being promoted live on data. Future maintainers no longer have to reverse-engineer the layout.

scripts/check-wiki-private-presence.test.ts

Two missing tests called out in the review:

  • node: null GraphQL response throws. Locks in the contract that a null node is treated as a failure (mode node-null), not a silent skip.
  • nameWithOwnerowner--repo slug conversion edge cases. it.each covering hyphen-in-owner (bfra-me/works), hyphen-in-repo (marcusrbrown/ha-config), dot-in-repo (bfra-me/.github), and uppercase normalization (MarcusRBrown/Polymarcusrbrown--poly). Catches a future "improvement" to the replacement that breaks one of the live conventions.

Plus a small NBC #2 distinction test that proves a mixed subprocess-threw + node-null failure set produces both mode labels in the error.

Verification

  • pnpm check-types, pnpm lint, pnpm test — green. 627 passed + 3 todo (+7 over PR feat(privacy): block merge-data when private wiki pages exist #3329's 620, all behavior-bearing).
  • actionlint .github/workflows/merge-data.yaml clean.
  • node -e "import('./scripts/check-wiki-private-presence.ts')" exits 0 under Node 24 strip-only.
  • node scripts/check-wiki-private-presence.ts against current main (no private entries) prints "no private wiki leaks detected", exits 0.

Deferred (still tracked)

The three larger design follow-ups from #3329's review remain open:

The single Fro Bot review item NOT addressed here is missing test #3 (comment near .replace(/\.md$/i, '') documenting the strict stem extraction) — that fits naturally with the #3328 work since both touch slug-variant handling.

Refs PR #3329.

Four non-blocking concerns from review:
- Parameterize the GraphQL node_id call (removes injection surface)
- Distinguish subprocess-threw from node-null failures in error output
- Normalize canonical slug to lowercase at storage
- Document the dual-checkout rationale in merge-data.yaml

Plus two missing tests:
- node:null GraphQL response → throws (contract lock-in)
- nameWithOwner → owner--repo slug conversion edge cases
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: PASS

The parameterized GraphQL call closes the injection surface cleanly — the query string is now a constant and node_id rides through execFileSync argv with no shell in the path. That's the right shape. Failure-mode labeling (subprocess-threw vs node-null) gives operators a remediation hint instead of a grep target, and lowercase normalization at storage matches the comparison side. Tests track each change.

Blocking issues

None.

Non-blocking concerns

  • scripts/check-wiki-private-presence.ts:84SlugResolutionResult.failures is typed string[] and commented "always empty on success; throws if non-empty". Since the throw path is the only way failures escape, the field is now structurally dead — it's always [] on return. Either drop it from the interface or re-type to ResolutionFailure[] and surface it for callers who want to introspect without parsing the error message. Minor — not worth holding the merge.
  • scripts/check-wiki-private-presence.ts:114gh api -F nodeId=<value> performs type coercion (numeric strings → numbers, true/false → booleans). Real GitHub node IDs are opaque base64-ish strings so this is fine in practice, and the upstream assertReposFile schema is the real gate. If you want belt-and-suspenders against a future schema regression, --field nodeId=... (long form) forces string. Defense-in-depth, not a defect.
  • .replace('/', '--') only replaces the first slash. GitHub nameWithOwner is owner/repo with exactly one slash, so this is correct — but the assumption is load-bearing. A single-line comment naming the invariant would help future maintainers more than another test would.

Missing tests

None for the changes in this PR. The deferred test #3 (comment on .replace(/\.md$/i, '')) is correctly punted to #3328 where slug-variant handling lives.

Risk assessment

LOW. The change reduces attack surface (GraphQL injection gone) and improves operator ergonomics (labeled failure modes, normalized storage). Behavior is fail-closed on resolution failure, which preserves the v1 gate's safety property. The dual-checkout comment in merge-data.yaml is documentation-only. No new dependencies, no permissions changes, no untrusted input flowing into run: blocks.

The bypass tracked in #3327 (metadata tampering) and the stderr re-leak in #3326 remain open — correctly scoped out of this PR.


Run Summary
Field Value
Event pull_request
Repository fro-bot/.github
Run ID 26143106506
Cache hit
Session ses_1bc29d7c3ffeJwtShkURyWB6Hf

@marcusrbrown marcusrbrown merged commit 73defc5 into main May 20, 2026
12 checks passed
@marcusrbrown marcusrbrown deleted the chore/private-wiki-gate-nbcs branch May 20, 2026 05:27
marcusrbrown added a commit that referenced this pull request May 20, 2026
…oncile (#3338)

* chore(privacy): apply review polish to private-wiki gate

Three follow-up nits from PR #3330 review:
- Drop SlugResolutionResult.failures (structurally dead; throw is the only failure path)
- Switch GraphQL nodeId arg to --field (forces string, avoids type coercion)
- Document the single-slash nameWithOwner invariant

* feat(privacy): detect public-to-private visibility transitions in reconcile

Reconcile probe now compares each tracked entry's stored `private`
flag against the probe result. A `false -> true` flip (including via
`access-lost`, which fails closed as private per Unit 2 semantics)
queues a `reconcile:visibility-transition` integrity-alert issue.

The issue identifies the entry by `node_id` only in both title and
body. Operators look up the canonical owner/repo locally via the
operator tool (deferred to a later unit). This is stricter than the
plan's permissive wording (which allowed canonical owner/name in the
body when the probe response still had it) and matches the always-
redacted-everywhere privacy posture.

Private-to-public transitions emit no alert; the next mutator write
recanonicalizes the entry. Newcomers (no stored `private` field)
get no alert; that is an initial categorization, not a flip.

Dedup-before-create matches the maybeCreateStaleDivergenceAlert pattern:
an existing open issue with the same node_id in the title suppresses a
new one. Auto-close is intentionally not wired — the operator closes
the issue manually after acting.

Adds reconcile:visibility-transition label to .github/settings.yml.

* fix(privacy): make visibility-transition alerts durable across transient failures

Round-2 review remediation for the Unit-9 visibility-transition path.

Durability (review P1):
- Fail open on dedup-list failure: if paginate(issues.listForRepo) throws,
  proceed with issue creation instead of silently dropping the alert.
- Pre-flight label creation: ensure reconcile:visibility-transition and
  reconcile:integrity-alert exist in the live repo before issues.create;
  defends against the Update-Repo-Settings propagation race.
- Drop labels gracefully if their createLabel call 422s — the alert still
  ships under whatever labels do exist.

Operator UX (review P1):
- Replace the dangling scripts/resolve-private.ts reference in the issue
  body with an inline `gh api graphql` command operators can paste
  directly into a terminal.

Correctness + types (review P2 safe_auto):
- runIssueQueue success counters: explicit switch over IssueQueueEntry
  kinds with a never exhaustiveness guard; visibility-transition no
  longer inflates the rollup counter.
- Dedup uses exact-title equality (functionally equivalent to NODE_ID
  marker match given the title is derived entirely from node_id), not
  substring matching — closes the false-positive/false-negative window.
- Replace the double-cast `as unknown as OpenIssueEntry[]` with an
  Octokit-derived type alias.
- Drop `as Map<string, {status: string}> as never` from test fixtures
  by typing makeProbeMap to the real RepoStatusProbe shape.
- Stale comment cleanup; minor doc drift in NBC #2's neighborhood.

Test coverage (review P2):
- Rendering test pins R15 privacy guarantee: node_id only in title and
  body, no canonical-slug separator (`owner--repo` pattern).
- Dedup branch test: existing open issue with matching node_id title
  suppresses new issues.create.
- Durability tests: paginate-throws and label-404 paths both still
  result in issues.create being attempted.

* fix(privacy): filter visibility-transition labels to confirmed-usable set

Round-3 review remediation. The previous round's 'drop labels gracefully'
claim was incomplete — ensureLabelExists swallowed exceptions but the
caller still passed the full label list to issues.create, so a transient
getLabel/createLabel failure still produced a 422 and dropped the alert.

- ensureLabelExists returns a Set<string> of confirmed-usable labels;
  non-404 getLabel and non-422 createLabel failures exclude that label.
- runIssueQueue filters the issue payload to only confirmed labels.
- All-labels-unconfirmed edge case ships unlabeled rather than dropping
  the alert.

Tighten test coverage to actually exercise the contract:
- RED-against-old-code dedup test using a victim node_id that is a strict
  prefix of an existing open-issue title. Substring dedup would suppress;
  exact-title dedup creates the new issue.
- Label preflight tests now assert call order (getLabel + createLabel
  before issues.create) and inspect issues.create's labels payload.
- Race vs transient createLabel split into two cases: 422 keeps the
  label, 500 excludes it.
- Privacy regex broadens to catch dot-prefixed slugs like
  fro-bot--.github.

Defense-in-depth at the schema boundary: node_id must match
^[\w\-+/=]+$. Rejected at metadata read time so the operator
copy-paste in the issue body can't be coerced into a different shell
command by a malformed entry.

* chore(privacy): document schema-render contract for node_id interpolation

Fro Bot review polish on the visibility-transition path:

- Cross-reference comment between the node_id regex in scripts/schemas.ts
  and the inline gh-api-graphql interpolation in renderVisibilityTransitionIssue
  so future maintainers see the schema invariant the render site depends on.
- Generic slash-form privacy regression pin in the rendering test catches
  any future renderer change that introduces a canonical owner/repo mention
  beyond the specific fixture string already asserted. Pattern uses a
  negative lookahead to exclude the prose placeholder 'owner/repo' that
  appears legitimately in the body's operator instructions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants