Skip to content

fix(agent): exclude repo identifier from scenario-summary guard - #625

Merged
JSONbored merged 5 commits into
JSONbored:mainfrom
philluiz2323:fix/scenario-summary-repo-name-guard
Jun 13, 2026
Merged

fix(agent): exclude repo identifier from scenario-summary guard#625
JSONbored merged 5 commits into
JSONbored:mainfrom
philluiz2323:fix/scenario-summary-repo-name-guard

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

Closes #624.

assertPublicSummaryClean serialized the entire summary and rejected it on FORBIDDEN_PUBLIC_LANGUAGE, but repoFullName is passed through un-sanitized (it is the repo identifier, not rendered content). So a legitimately named repo — and hotkey/coldkey/wallet are literal Bittensor protocol terms — made renderPublicScenarioSummary throw, failing the public scenario summary entirely. The guard's own /* v8 ignore … all fields are sanitized */ rationale was false for repoFullName.

Change

  • Scan only the rendered free-text content; destructure out repoFullName/generatedAt (structural identifiers) before the forbidden-language check.
  • Add a regression test: repoFullName: "octo/hotkey-wallet" renders without throwing and preserves the name.

Verification

  • scenario-summary.test.ts 9/9 (the new case throws on the old code); tsc --noEmit clean; full suite green; branch coverage holds (>= 97% gate).

Same accepted class as #457 (sanitizer breaking valid repo names like wallet-adapter), different surface: a public-output guard that hard-threw on the un-sanitized repoFullName.

assertPublicSummaryClean serialized the whole summary -- including the un-sanitized repoFullName -- and threw on FORBIDDEN_PUBLIC_LANGUAGE, so a legitimately named repo (hotkey/coldkey/wallet are Bittensor terms) made renderPublicScenarioSummary throw and fail entirely. Scan only the rendered free-text content; repoFullName/generatedAt are structural identifiers, not sanitized output.
@ghost

ghost commented Jun 12, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR updates the public summary guard to ignore structural identifiers (repoFullName, generatedAt) when checking for forbidden language, and adds a test ensuring repo names containing forbidden terms no longer cause a guard failure. The change is straightforward, type‑safe, and adds appropriate coverage.

Suggestions

  • Consider adding a test that verifies the guard still throws when a forbidden term appears in a rendered free‑text field (e.g., headline).
  • Document that any new fields added to PublicScenarioSummary must be sanitized before inclusion, as the guard now only checks renderedContent.

Worth double-checking

  • Future fields added to the summary might be omitted from the guard if not included in renderedContent; ensure they are sanitized.
  • UI components displaying repoFullName should not rely on this guard for safety; they must handle any potentially sensitive identifiers appropriately.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR modifies the public summary guard to exclude structural identifiers (repoFullName, generatedAt) from forbidden language checks, as they are not sanitized content and may legitimately contain terms like 'private context'. A test confirms repo names with such terms don't trigger the guard. This aligns with the public/private boundary by treating repo metadata as public GitHub data, not private context information.

Suggestions

  • Add a comment noting that future structural identifier fields should also be excluded from the guard.
  • Verify existing tests still pass for forbidden language in rendered content (though unchanged in diff).

Worth double-checking

  • If repoFullName/generatedAt ever contained actual private data (unlikely, as they're from public GitHub), the guard would miss it.
  • Future non-structural fields added to the summary might bypass the guard if not considered 'rendered content'.

@ghost

ghost commented Jun 12, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #625 is no longer open. No action.

💰 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.

@superagent-security

Copy link
Copy Markdown
Contributor

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

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔎 Gittensory inline review — 1 line-level note(s). Advisory only; a maintainer merges. See the summary comment for the full review.

Comment thread src/scenarios/scenario-summary.ts

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@philluiz2323 this is ready.

A few notes:

  • The fix is scoped to structural identifiers instead of weakening the sanitizer for rendered public text.
  • The regression test covers the real failure mode: a legitimate repo name containing protocol terms should not make the scenario summary throw.
  • No broad sanitizer behavior changed.

No code changes requested.

@dosubot dosubot Bot added the lgtm label Jun 13, 2026
@JSONbored
JSONbored merged commit 9233586 into JSONbored:main Jun 13, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

2 participants