Skip to content

fix: escape HTML comments in review skill, add output schema to agent def - #1111

Closed
waynesun09 wants to merge 3 commits into
mainfrom
fix-scan-hidden-comment
Closed

fix: escape HTML comments in review skill, add output schema to agent def#1111
waynesun09 wants to merge 3 commits into
mainfrom
fix-scan-hidden-comment

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

  • Escape literal HTML comment patterns (<!-- ... -->) in the pr-review SKILL.md that triggered fullsend scan context high-severity hidden_html_comment findings. The scanner does line-by-line matching and doesn't respect code fences.
  • Add the full pipeline output JSON schema (required fields per action, finding object structure, validation behavior) to the review agent definition. Previously this was only documented in the pr-review skill and the JSON schema file, so the agent lacked visibility into what the harness validates.

Test plan

  • fullsend scan context passes clean on pr-review SKILL.md
  • Review agent produces valid agent-result.json on next PR review run

The fullsend context scanner flags literal HTML comments in context
files as hidden_content injection. The pr-review SKILL.md and review
agent definition contained raw `<!-- ... -->` examples that triggered
a high-severity finding. Replace them with decomposed delimiter
descriptions that convey the same format without matching the scanner
pattern. Also adds the expected review comment format to the review
agent definition so it is not only documented in the skill.

Signed-off-by: Wayne Sun <gsun@redhat.com>
The review agent definition now documents the full JSON schema for
agent-result.json including required fields per action, finding object
structure, and validation behavior. Previously this was only in the
pr-review skill and the JSON schema file, causing agents to miss
required fields and fail harness validation.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

Site preview

Preview: https://05c082bf-site.fullsend-ai.workers.dev

Commit: 5efb0ee936ba271e7ac948ca45603a0cedd8a8ab

- Normalize double-spaces to single-spaces in the Head SHA example
  in SKILL.md to match the original format
- Restore literal HTML comment in review.md failure output code fence
  (scanner skips review.md — it's not a known context file)
- Mark schemas/review-result.schema.json as source of truth in the
  schema documentation section

Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Low

  • [incomplete-fix] internal/scaffold/fullsend-repo/agents/review.md:184 — The literal HTML comment pattern inside the "Failure output" code fence was not escaped. The PR fixes this pattern in SKILL.md to avoid hidden_html_comment scanner findings, but this instance in agents/review.md (a file already modified in this PR) still uses the literal form. If the scanner doesn't respect code fences, this will continue to trigger.
    Remediation: Apply the same [open]/[close] workaround used in SKILL.md step 6 to the failure output example in agents/review.md.

  • [incomplete-fix] .claude/skills/pr-review/SKILL.md:235 — Three non-scaffold copies of SKILL.md (.claude/skills/pr-review/SKILL.md, skills/pr-review/SKILL.md, .cursor/skills/pr-review/SKILL.md) still contain the literal HTML comment pattern at lines 235 and 425. These copies are not modified by this PR and will continue to trigger scanner findings.
    Remediation: Apply the same escaping fix to the three deployed copies, or sync them from the scaffold template after this PR merges.

**Required fields per action:**

| Action | Required fields |
|-------------------|------------------------------------------|

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] incomplete-fix

The literal HTML comment pattern inside the Failure output code fence was not escaped. The PR fixes this pattern in SKILL.md but misses this instance in agents/review.md, which is also modified in this PR. If the scanner does not respect code fences, this will still trigger a hidden_html_comment finding.

Suggested fix: Apply the same [open]/[close] workaround used in SKILL.md step 6 to the failure output example in agents/review.md.

@fullsend-ai-review

Copy link
Copy Markdown

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

  • #1114 — The literal HTML comment pattern inside the Failure output code fence was not escaped. The PR fixes this pattern in SKILL.md but misses this instance in agents/review.md, which is also modified in this PR. If the scanner does not respect code fences, this will still trigger a hidden_html_comment finding.
  • #1115 — Three non-scaffold copies of SKILL.md (.claude/skills/, skills/, .cursor/skills/) still contain the literal HTML comment pattern at lines 235 and 425. These copies are not modified by this PR and will continue to trigger scanner findings.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label May 18, 2026
waynesun09 added a commit that referenced this pull request May 18, 2026
Merge content from #1111 into this PR:
- Escape literal HTML comment patterns in pr-review SKILL.md that
  trigger fullsend scan context hidden_html_comment findings
- Add JSON schema tables (fields, types, constraints, required-per-action)
  to the agent definition alongside the existing jq examples
- Mark schemas/review-result.schema.json as source of truth
- Replace inline HTML comment reference in SKILL.md constraints with
  back-reference to step 6

Supersedes #1111.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@waynesun09

Copy link
Copy Markdown
Member Author

Superseded by #1112 — all content from this PR (scan escape fix, schema tables, source-of-truth annotation) has been merged into #1112's branch.

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

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant