Skip to content

fix(content-policy): bind pinned installer evidence#2086

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-bypass-in-pinned-installer-policy
Open

fix(content-policy): bind pinned installer evidence#2086
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-bypass-in-pinned-installer-policy

Conversation

@JSONbored

@JSONbored JSONbored commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Motivation

  • Close a bypass where any pinned GitHub script URL in submitted metadata could silence the mutable_script_install_source check even when the pinned URL was unrelated to the cloned repo or the executed local script.
  • Require stronger, bound evidence so reviewers can trust that a local installer executed after a git clone is pinned and actually checked out before execution.

Description

  • Enhance githubSourceRef to include owner and repo metadata and add githubRepoRef/githubRepoKey helpers to identify repository origins for submitted URLs in scripts/ci/validate-content-policy.mjs.
  • Add helpers to detect cloned GitHub repos (collectGitCloneRepos), normalize and collect local installer script paths (collectLocalScriptInstallRefs / normalizeScriptPath), detect checkout of a pinned commit (checkoutCommitIndex), and validate bound evidence (hasBoundImmutableGithubScriptEvidence).
  • Replace the prior global pinned-URL check with a bound check so mutable_script_install_source is suppressed only when a pinned script URL matches the cloned repo, matches the executed script path, and the install instructions explicitly check out the pinned commit before running the script in scripts/ci/validate-content-policy.mjs.
  • Add regression tests to tests/content-policy-validation.test.ts covering unrelated pinned script evidence, wrong script path evidence, missing checkout of the pinned commit, and the valid same-repo/same-script/pinned-checkout case.

Testing

  • Ran pnpm exec vitest run tests/content-policy-validation.test.ts and all tests passed (19 tests, 0 failures).
  • Ran pnpm validate:content:strict which validated content (912 files) and passed.
  • Ran git diff --check to ensure no trailing whitespace/format issues and the check passed.

Codex Task

Summary by CodeRabbit

  • Tests

    • Added test cases for stricter validation of installation script sources and pinned commit verification.
  • Chores

    • Enhanced validation logic to better detect and enforce immutable script sources with improved normalization and evidence-binding mechanisms.

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR tightens content policy validation for GitHub scripts by replacing a simple URL-based immutability check with a contextual evidence-binding mechanism that verifies pinned scripts are tied to git clone and commit checkout commands in installer execution flows.

Changes

Immutable Script Binding Validation

Layer / File(s) Summary
URL normalization for GitHub references
scripts/ci/validate-content-policy.mjs
GitHub source URLs extracted from both raw.githubusercontent.com and github.com are normalized by lowercasing the owner, stripping .git suffix, and lowercasing the repo name for consistent comparison.
Evidence-binding helpers and risk signal integration
scripts/ci/validate-content-policy.mjs
New helper functions parse GitHub repo references from git clone commands, normalize locally referenced installer script paths, detect git checkout to pinned commit SHAs, and compute whether immutable evidence is bound to the cloned repo context. The old isImmutableGithubScriptSourceUrl function is removed. The risk signal gate in addContentRiskSignals is updated to use hasBoundImmutableGithubScriptEvidence instead of checking source URLs alone.
Test validation of stricter binding enforcement
tests/content-policy-validation.test.ts
Three new Vitest cases verify that immutable pinned evidence must match both the installer script path and the commit checkout referenced in install instructions; each test confirms the mutable_script_install_source review flag is emitted when evidence is misaligned, references unrelated revisions, or when checkout does not match pinned evidence.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • JSONbored/awesome-claude#2018: Both PRs update content policy validation in the same files to enforce and verify immutable, commit-pinned GitHub evidence bound to cloned local installer script execution flows.

Suggested labels

gittensory:reviewed

Poem

🐰 Hop, hop, normalization!
GitHub refs now shine so bright,
Evidence binds tight to scripts—
No more mutable sleight-of-hand,
Just immutable proof in every command! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main change: binding pinned installer evidence to strengthen content policy validation.
Description check ✅ Passed The description provides thorough context including motivation, detailed technical changes, testing methodology, and verification results, though it follows a custom format rather than the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-bypass-in-pinned-installer-policy

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gittensory

gittensory Bot commented Jun 11, 2026

Copy link
Copy Markdown

Important

Gittensory found maintainer review notes

Public GitHub metadata was checked for review readiness. Gittensor-specific context appears only when confirmed.

Readiness score: 93/100

Signal Result Evidence Action
Linked issue ⚠️ Missing No linked issue or no-issue rationale found. Explain no-issue PR.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found. No action.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:L. No action.
Validation evidence ✅ 25/25 PR body includes validation/test evidence. No action.
Open PR queue ✅ 10/10 0 open PR(s), 0 likely reviewable. No action.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 62 PR(s), 347 issue(s). No action.
Gate result ✅ Passing No configured blocker found. No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 62 PR(s), 347 issue(s).
  • PR-specific overlap: none found.
Maintainer notes
  • No linked issue detected: The planned PR does not reference a closing issue or explicit linked issue number.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Re-run Gittensory review

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. Learn more about Gittensor contribution workflows.

@gittensory gittensory Bot added the gittensory:reviewed Gittensor contributor context label Jun 11, 2026
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
tests/content-policy-validation.test.ts (1)

290-335: 💤 Low value

Consider adding a test case for git switch --detach variant.

The checkoutCommitIndex function supports both git checkout and git switch --detach for checking out a pinned commit, but the test suite only exercises the git checkout path. Adding a test with git switch --detach ${revision} would improve confidence in that code path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/content-policy-validation.test.ts` around lines 290 - 335, Add a new
test variant in tests/content-policy-validation.test.ts mirroring the "allows
cloned local scripts with immutable script source evidence" case but use the
alternative checkout command supported by checkoutCommitIndex: replace
occurrences of "git checkout ${revision}" in both the installCommand and the
bash snippet with "git switch --detach ${revision}" (give the new it(...) a
distinct name like "allows cloned local scripts with immutable script source
evidence using git switch --detach"); call runContentPolicy the same way (e.g.,
with "same_repo_direct" or a new scenario name if needed), then assert
result.status is 0 and that the output.reviewFlags does not contain { id:
"mutable_script_install_source" } just like the original test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/content-policy-validation.test.ts`:
- Around line 290-335: Add a new test variant in
tests/content-policy-validation.test.ts mirroring the "allows cloned local
scripts with immutable script source evidence" case but use the alternative
checkout command supported by checkoutCommitIndex: replace occurrences of "git
checkout ${revision}" in both the installCommand and the bash snippet with "git
switch --detach ${revision}" (give the new it(...) a distinct name like "allows
cloned local scripts with immutable script source evidence using git switch
--detach"); call runContentPolicy the same way (e.g., with "same_repo_direct" or
a new scenario name if needed), then assert result.status is 0 and that the
output.reviewFlags does not contain { id: "mutable_script_install_source" } just
like the original test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9a24a1f5-931a-465e-8c9f-e9e49be820ef

📥 Commits

Reviewing files that changed from the base of the PR and between ec30bc5 and e291a38.

📒 Files selected for processing (2)
  • scripts/ci/validate-content-policy.mjs
  • tests/content-policy-validation.test.ts

@JSONbored JSONbored self-assigned this Jun 11, 2026
@JSONbored JSONbored closed this Jun 11, 2026
@JSONbored JSONbored reopened this Jun 11, 2026
@reviewwed

reviewwed Bot commented Jun 11, 2026

Copy link
Copy Markdown

🔍 Maintainer advisory review

Reviewed 2 changed file(s) — two independent AI reviewers. This is an advisory review — it does not merge or close the PR.

Suggested action: 🛠️ Request changes. (reviewers split: merge / request changes) Address the items below before merging.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The change correctly implements the intended binding of immutable script evidence to cloned install instructions, replaces the previous simple check with a more precise validation, and adds comprehensive tests for the new logic. No new security concerns are introduced; the code only performs regex parsing on untrusted text without executing commands or making network calls. The implementation follows existing patterns, and the added helper functions are well‑encapsulated. The only caution is a potential increase in false‑positives for submissions that fetch scripts without an explicit checkout, which may affect existing entries, but this aligns with the stricter policy intent.

Suggestions

  • Add documentation/comments describing the expected install command patterns and the rationale for the stricter binding.
  • Consider adding tests for edge‑case clone commands (e.g., with flags like --depth, --single-branch) and for checkout via git switch --detach <commit>.
  • Review existing content entries to ensure they still pass the updated policy or update them accordingly.

Worth double-checking

  • Possible regression where previously accepted submissions that use direct script URLs without explicit checkout may now be flagged as mutable.
  • Regex patterns could miss uncommon git clone or checkout syntaxes, leading to false positives.

Reviewer B · mistral-small-3.1-24b-instruct — recommends 🛠️ request changes
The change introduces a new validation mechanism to ensure that pinned installer evidence is correctly bound to the cloned repository and the script being executed. It modifies the content policy validation script and adds corresponding tests.

Suggestions

  • Consider adding more detailed comments to explain the new functions and their purpose.
  • Ensure that the regular expressions used in the new functions are optimized to avoid potential ReDoS (Regular Expression Denial of Service) attacks.
  • Add more edge case tests to cover scenarios where the input might be malformed or unexpected.

Worth double-checking

  • The new regular expressions and string manipulations could introduce subtle bugs if not thoroughly tested.
  • There is a risk of false positives or negatives in the validation logic if the patterns do not cover all possible cases.
  • The change modifies critical validation logic, which could affect the correctness of the content build/validation/registry if not properly tested.

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

Labels

aardvark codex gittensory:reviewed Gittensor contributor context size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant