Skip to content

fix(review): keep screenshots allowlist-bound - #4990

Merged
JSONbored merged 1 commit into
mainfrom
codex/propose-fix-for-screenshot-allowlist-bypass
Jul 12, 2026
Merged

fix(review): keep screenshots allowlist-bound#4990
JSONbored merged 1 commit into
mainfrom
codex/propose-fix-for-screenshot-allowlist-bypass

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • A prior refactor added screenshots to the converged features: resolver using the default standard precedence, which let a repo's .gittensory.yml features.screenshots: true bypass the operator GITTENSORY_REVIEW_REPOS allowlist and trigger costly server-side browser captures.

Description

  • Keep screenshots under allowlistRequired by adding screenshots: "allowlistRequired" to FEATURE_MODE in src/review/feature-activation.ts so a manifest cannot force it ON outside the operator allowlist.
  • Update the feature-mode documentation/comments in src/review/feature-activation.ts to call out that screenshots launches browser rendering and stores/publicly-embedded images, and therefore must remain allowlist-bound.
  • Add regression tests in test/unit/feature-activation.test.ts and test/unit/visual-wire.test.ts that assert an unallowlisted repo with features.screenshots: true cannot activate screenshots and that allowlisted repos still default ON or can opt out.
  • Regenerate Cloudflare worker runtime types (worker-configuration.d.ts) after the local cf-typegen drift check reported stale generated output.

Testing

  • Ran the focused unit suites with npx vitest run test/unit/feature-activation.test.ts test/unit/visual-wire.test.ts, and both test files passed (all tests succeeded).
  • Ran npm run typecheck and npm run cf-typegen:check (and regenerated worker-configuration.d.ts), both succeeded locally.
  • Attempted the full gate via npm run test:ci; the run exercised the suite but stopped on unrelated, pre-existing failures in test/unit/queue-5.test.ts (these are unrelated to this change and prevented a full green test:ci).
  • npm audit --audit-level=moderate could not complete due to the registry endpoint returning 403 Forbidden in this environment.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.18%. Comparing base (a21a8ad) to head (2fd6a31).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4990   +/-   ##
=======================================
  Coverage   94.18%   94.18%           
=======================================
  Files         468      468           
  Lines       39608    39608           
  Branches    14448    14448           
=======================================
  Hits        37306    37306           
  Misses       1646     1646           
  Partials      656      656           
Flag Coverage Δ
shard-1 46.47% <ø> (-0.01%) ⬇️
shard-2 33.58% <ø> (-0.27%) ⬇️
shard-3 31.39% <ø> (-0.82%) ⬇️
shard-4 32.68% <ø> (+0.72%) ⬆️
shard-5 33.21% <ø> (-0.10%) ⬇️
shard-6 45.18% <ø> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/feature-activation.ts 100.00% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored closed this Jul 11, 2026
@JSONbored JSONbored reopened this Jul 11, 2026
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-12 08:23:19 UTC

3 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This closes a real allowlist-bypass: `screenshots` was resolving under the default `"standard"` mode, letting a repo's own `.gittensory.yml` force browser-based screenshot capture ON even when the repo isn't in `GITTENSORY_REVIEW_REPOS`. Adding `screenshots: "allowlistRequired"` to `FEATURE_MODE` in feature-activation.ts:105 routes it through the same `allowlisted && override !== false` arithmetic already used for `grounding`, which is the correct fix at the source (resolveFeatureActivation), not a symptom patch. The doc comments and both test files (feature-activation.test.ts, visual-wire.test.ts) are updated consistently, and the new/changed assertions exercise the real `resolveConvergedFeature` call path — not a fabricated state. The worker-configuration.d.ts diff is pure generated-artifact churn from a `cf-typegen` regen, unrelated to the fix but explained and low-risk.

Nits — 4 non-blocking
  • The PR description doesn't cite an open issue this fix closes — only historical refs (fix(review): features.safety is per-repo disableable via .gittensory.yml even when the operator enabled it globally #2269, Unify per-feature *-wire.ts activation behind one shared resolver #4616) for context; per this repo's convention, contributor PRs should link the issue they close even for a bug/security fix like this one.
  • Bundling the large generated worker-configuration.d.ts regen with the behavioral fix makes the diff noisier to scan even though it carries no independent risk — consider splitting generated-artifact updates into their own PR/commit going forward.
  • Consider a short changelog/release-note line calling out that screenshots was previously allowlist-bypassable, since operators who noticed unexpected browser-capture costs will want to know this was the root cause.
  • The doc comment at feature-activation.ts:47-60 now lists three `allowlistRequired` features' worth of rationale inline — as more features accumulate this shape, consider a per-feature rationale map instead of prose to keep it scannable.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 45 registered-repo PR(s), 37 merged, 411 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 45 PR(s), 411 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
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, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 45 PR(s), 411 issue(s).
  • PR-specific overlap: none found.
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.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory mention with a real question is routed to the closest matching read-only command automatically -- no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 11, 2026
@JSONbored
JSONbored force-pushed the codex/propose-fix-for-screenshot-allowlist-bypass branch from b88ace3 to 2fd6a31 Compare July 11, 2026 13:09
@JSONbored
JSONbored merged commit db28d8f into main Jul 12, 2026
18 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-screenshot-allowlist-bypass branch July 12, 2026 08:26
JSONbored added a commit that referenced this pull request Jul 12, 2026
…x, chat-qa test fix (#5201)

* docs(selfhost): document the claude code token secret file + fix stale screenshots note

Two drift fixes found while auditing the recent secrets-migration and
screenshot-allowlist changes:

- The security and AI-providers docs pages only described the inline
  .env path for CLAUDE_CODE_OAUTH_TOKEN; add the secrets/*.txt file
  alternative now that docker-compose.yml supports it.
- config/examples/gittensory.full.yml and .gittensory.yml.example both
  still called `screenshots` a "plain symmetric override" -- #4990 made
  it allowlist-required (force-off only outside the rollout allowlist),
  the same asymmetry as safety/grounding. Update both mirrored comment
  blocks to match.

* fix(ci): build gittensory-engine before UI typecheck on ui-only PR diffs

validate-code's "Build engine package" step only ran on push, or when
backend/engine changed -- not when only ui changed. But the very next
"UI typecheck" step (gated separately on ui) transitively needs that
build: apps/gittensory-ui's tsconfig pulls in src/mcp/local-write-tools.ts,
which imports @jsonbored/gittensory-engine, whose dist/ is gitignored
and only exists after this step runs. Any PR touching only apps/gittensory-ui/**
(a docs-only page edit, for example) hit TS2307 here with no code being
wrong -- confirmed live on PR #5201's own validate-code run. Add the
missing `|| needs.changes.outputs.ui == 'true'` condition.

* fix(test): exercise the real lane-signal redaction path in ai-chat-qa

#5149 added PRIVATE_LANE_SIGNAL_PATTERN and correctly wired it into
redactGroundingText, applied to objective/summary/publicSafeSummary/
freshnessWarnings -- the only fields compactChatSignalBundle() ever
reads. But its own new integration test overrode action.why instead,
a field that function never reads at all (by design: raw action
rationale/blockedBy is omitted entirely from the chat grounding
bundle, not merely redacted -- see the comment above
PRIVATE_DECISION_BLOCKER_PATTERN). The override never reached the
prompt, so neither assertion exercised the new pattern; it happened
to still report a false pass shape until CI caught the real failure
downstream. Point the override at publicSafeSummary, the field that
actually flows into the prompt, so the test validates what it claims to.

No src/ change: redactGroundingText and its regex were already correct,
proven by the adjacent pure-function unit test a few lines down in the
same file, which was passing throughout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant