fix(review): take the artifact gate's visibility only from the view facts

[kyle-sexton]

Requested by Kyle · project thread

Refs: #6951

The read-pr.sh host check the issue names is not done here; it is a follow-up after #7081 lands.

Summary

Before: most of what the issue asked for already landed in #7026, after the issue was filed. /source-control:pull-request view --out facts.json looks the repository's visibility up over REST and writes it into the facts, and SKILL.md runs the artifact gate as digest-policy.mjs --publish-gate --facts <dir>/facts.json. One gap was left: the gate still accepted a visibility typed as its first argument (--publish-gate PUBLIC). A model that skipped --facts could still feed the gate its own reading of the repository.

After: --publish-gate accepts only --facts <facts.json> [--explicit]. Any typed visibility exits 2 with the usage line and no result. The artifact medium now gates only on the value the source-control owner looked up, just as the hosted medium only gates on the value publish-hosted.mjs looks up itself.

Fix

• digest-policy.mjs: gateMain requires --facts <file> and takes the visibility from factsVisibility(file). A missing, unreadable or empty value still gates as UNKNOWN, which goes to file. The usage line and header comment changed to match.
• SKILL.md step 4: one clause saying the gate takes no visibility argument and reads the value the view action looked up.
• Tests: the old "CLI reads the diff on stdin" case passed typed visibilities. It is now a refusal case: public, PUBLIC, PUBLIC --explicit, PUBLIC --facts, --explicit and no arguments each exit 2 with empty stdout. The --facts case adds a lowercase value, a PRIVATE value without --explicit, and an unknown trailing flag.
• Changelog fragment for review (patch, Security).
• ADR 0060: the review skill names no gh command. Visibility comes from the /source-control:pull-request view action, so no owner-side addition was needed.

Where the issue left a detail open: I did not copy publish-hosted.mjs's host check (gh auth status --hostname, ignoring enterprise env tokens) into read-pr.sh. The host the view action's visibility lookup asks comes from the PR URL that gh pr view already fetched from that host. The check would only matter for a non-github.com host reached through an enterprise env token. Adding it means editing read-pr.sh, which open draft #7081 is rewriting. It suits a small follow-up after #7081 lands, if wanted.

Verification

• node --test plugins/review/tests/explain-change.test.mjs: 103 tests, 102 pass, 1 skipped (existing platform skip), 0 fail.
• Before/after on the same input: echo diff | node <main's digest-policy.mjs> --publish-gate PUBLIC printed medium: artifact, exit 0. The new script prints the usage line, exit 2.
• scripts/affected-tests.sh --base origin/main --run: 2 shell suites pass. It selects explain-change.test.mjs but does not run it (a node lane, exit 3 for that reason). I ran that suite directly, above.
• scripts/check-changelog-fragments.sh --check and --check-required origin/main: pass. scripts/check-changelog-parity.sh --check: pass.
• markdownlint-cli2 on SKILL.md and the fragment: 0 issues. biome check on the two .mjs files reports 4 findings, all on lines this PR does not touch.

Related

• refactor(skills): route generic skills' forge work through source-control actions #7026 moved explain-change's forge reads, the visibility lookup included, behind /source-control:pull-request view. That covered the issue's main ask.
• fix(review): publish-hosted looks up repository visibility itself instead of trusting --repo-visibility #6748 / fix(review): publish-hosted looks up repository visibility itself #6783: the hosted-medium fix this issue mirrors.
• fix(source-control): read pull-request facts and diffs over REST in view #7081 (open draft, fix(review): explain-change gathers PR facts through REST so it works in cloud sessions #6750) moves view to REST. This PR does not depend on it and does not touch read-pr.sh. With fix(source-control): read pull-request facts and diffs over REST in view #7081, facts.json can also be produced in cloud sessions where GraphQL returns 403.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L8KdG7fWg2YrErVYWtohmR

---

Generated by Claude Code

---

Generated by Claude Code

[kyle-sexton]

ci-status is red because this PR is a draft, not because of the change. On a draft every lint and test lane skips (lint-repo, test-bash, test-node, test-python, check-skills, check-plugins), and the aggregate counts a skip as not passing. See .github/workflows/pr-require-checks.yml:507-513.

The lanes run once the PR is marked ready. That flip waits for Kyle, because it also starts the review lanes. Until then, the local runs listed under Verification in the PR body are the signal.

---

Generated by Claude Code

[github-actions]

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T03:46:29.386777Z	1821a9f	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 2m 16s —— View job

---

Review: fix(review): take the artifact gate's visibility only from the view facts (#7087)

• Gather context (PR body, diff, existing CLI/tests, REVIEW.md lane scope)
• Resolve team standards (docs/standards/README.md): no paved-path row, nothing to apply
• Read digest-policy.mjs (factsVisibility, gateMain, publishGate) and the shared lib/publish-gate.mjs it wraps
• Check SKILL.md step 4 wording against the new CLI contract
• Check explain-change.test.mjs coverage of the new refusal/accept paths
• Grep for any leftover caller still passing a typed visibility positional arg
• Post findings

Scope note: this repo runs a dedicated security lane (pr-review-security-hosted.yml), so per REVIEW.md's lane split this lane reports only correctness/design/tests/docs, not security impact — appropriate here since the change is itself a security hardening, not a new security-relevant surface.

What I checked:

• gateMain (digest-policy.mjs#L349-L359) now requires flag === "--facts" with a present, non-flag-shaped file argument, and rejects any trailing arg other than --explicit. Traced every new test case (public, PUBLIC, PUBLIC --explicit, PUBLIC --facts, --explicit, [], and the --force trailing-flag case) against the logic by hand — all match the asserted exit codes/stdout.
• factsVisibility (L340-L347) safely degrades to "UNKNOWN" on missing file, bad JSON, or a non-string/empty visibility key, consistent with publishGate's not-PUBLIC handling.
• Confirmed visibility is the exact key /source-control:pull-request view's read-pr.sh writes (plugins/source-control/skills/pull-request/scripts/read-pr.sh:217), already uppercased, so factsVisibility(...).toUpperCase() is a no-op there and only helps the lowercase-value test case.
• SKILL.md's step 4 wording matches the new signature and the --publish-gate --facts <dir>/facts.json [--explicit] invocation exactly.
• Grepped the repo for any other caller still passing a positional <VISIBILITY> to --publish-gate: none — only the diff's own removed lines matched.
• The changelog fragment (bump: patch, ### Security) matches the format of sibling fragments in .changes/review/.

I did not independently run node --test plugins/review/tests/explain-change.test.mjs — this lane's Bash tool required an approval step I can't grant, so the author-claimed "103 tests, 102 pass, 1 skipped" result in the PR body is unverified by me; I instead hand-traced the new/changed test cases against the updated gateMain logic above and they check out.

No correctness, test-coverage, or maintainability issues found at this lane's high-signal bar. No inline comments posted.

claude-lane-summary: lane=claude-review sha=1821a9f84e053d6cf84800896337b4dcfbeda5f8 run=38343088989

[claude]

Claude finished @kyle-sexton's task in 1m 27s —— View job

---

Security review complete

• Gather context (PR body, changed files)
• Review diff for security issues
• Validate any candidate findings
• Post final results

No security issues found.

This PR closes a real gap: digest-policy.mjs --publish-gate previously accepted a typed <VISIBILITY> argument, so a model that skipped --facts could feed the artifact publish gate its own (unverified) reading of the repository's visibility and potentially push a private repo's content to a claude.ai Artifact. The fix removes that path entirely — gateMain in digest-policy.mjs#L349-L359 now accepts only --facts <file> [--explicit] and exits 2 on anything else (including a bare visibility, --facts with no file, or trailing unknown flags).

I traced where the trusted value actually originates: read-pr.sh (read-pr.sh#L205-L217) derives host/repo_path from the PR's own URL and looks visibility up via gh api repos/$repo_path, never from PR-author-controlled text (title, body, diff). So facts.json's visibility field is attacker-resistant, and factsVisibility() in the new code reads only that field — the fix genuinely narrows the trust boundary rather than just relocating it.

Other checks:

• The --explicit opt-in path still requires a non-default medium.source per SKILL.md, unchanged by this diff.
• Test coverage (explain-change.test.mjs) now asserts every typed-visibility variant (public, PUBLIC, PUBLIC --explicit, PUBLIC --facts, --explicit, no args) is refused with exit 2 and empty stdout, plus the --facts path's lowercase/PRIVATE/explicit/unknown-flag cases.
• lib/publish-gate.mjs (shared) and publish-hosted.mjs are untouched here; the hosted medium already looks visibility up itself, consistent with the PR's stated scope.
• No injection, path-traversal, or secret-handling issues in the touched lines — factsVisibility only reads a visibility string field from a JSON file the skill itself names, with try/catch defaulting to UNKNOWN on any failure.

No instruction-surface deletions in this diff (no rule or guardrail was removed, only an unsafe input path to an existing gate).

Verified independently of the author's own write-up in the PR body.

claude-lane-summary: lane=claude-security-review sha=1821a9f84e053d6cf84800896337b4dcfbeda5f8 run=38343088171
· branch

[github-actions]

claude-security-review has reviewed this pull request through 1821a9f; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through 1821a9f; a later push is reviewed from there.