Enforce minimum create_issue body length in safe outputs schema and validator#38114
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
create_issue body length in safe outputs schema and validator
There was a problem hiding this comment.
Pull request overview
This PR tightens safe-output validation for create_issue by enforcing a minimum body length (20 chars) across the Go validation config, the tool JSON schemas used at runtime/compile time, and the JS validator tests to reduce low-signal/placeholder issue bodies.
Changes:
- Added
MinIssueBodyLength = 20and enforced it viaMinLengthoncreate_issue.bodyin the Go safe-outputs validation config. - Updated both compiler-embedded and runtime tool schemas to include
minLength: 20forcreate_issue.body. - Updated JS validator tests and Go sample fixtures to satisfy the new constraint; workflow lockfile was also regenerated with many unrelated changes.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_validation_config.go | Introduces MinIssueBodyLength and applies it to create_issue.body validation rules. |
| pkg/workflow/js/safe_outputs_tools.json | Adds minLength: 20 to the compiler-embedded create_issue tool schema. |
| actions/setup/js/safe_outputs_tools.json | Adds minLength: 20 to the runtime create_issue tool schema. |
| actions/setup/js/safe_output_type_validator.test.cjs | Updates test config and adds explicit create_issue minLength coverage + adjusts existing tests to use longer bodies. |
| pkg/workflow/samples_validation_test.go | Updates create_issue sample bodies to meet the new minimum length. |
| .github/workflows/daily-safeoutputs-git-simulator.lock.yml | Regenerates the lockfile, introducing broad workflow/version/behavior changes beyond the stated PR goal. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
| @@ -54,7 +55,7 @@ var ValidationConfig = map[string]TypeValidationConfig{ | |||
| DefaultMax: 1, | |||
| Fields: map[string]FieldValidation{ | |||
| "title": {Required: true, Type: "string", Sanitize: true, MaxLength: 128}, | |||
| "body": {Required: true, Type: "string", Sanitize: true, MaxLength: MaxBodyLength}, | |||
| "body": {Required: true, Type: "string", Sanitize: true, MaxLength: MaxBodyLength, MinLength: MinIssueBodyLength}, | |||
| @@ -75,9 +74,14 @@ jobs: | |||
| permissions: | |||
| actions: read | |||
| contents: read | |||
| env: | |||
| GH_AW_MAX_DAILY_AI_CREDITS: "500000" | |||
| outputs: | |||
| comment_id: "" | |||
| comment_repo: "" | |||
| daily_effective_workflow_exceeded: ${{ steps.daily-effective-workflow-guardrail.outputs.daily_effective_workflow_exceeded == 'true' }} | |||
| daily_effective_workflow_threshold: ${{ steps.daily-effective-workflow-guardrail.outputs.daily_effective_workflow_threshold || '' }} | |||
| daily_effective_workflow_total_effective_tokens: ${{ steps.daily-effective-workflow-guardrail.outputs.daily_effective_workflow_total_effective_tokens || '' }} | |||
| engine_id: ${{ steps.generate_aw_info.outputs.engine_id }} | |||
| lockdown_check_failed: ${{ steps.generate_aw_info.outputs.lockdown_check_failed == 'true' }} | |||
| model: ${{ steps.generate_aw_info.outputs.model }} | |||
| @@ -87,34 +91,41 @@ jobs: | |||
| setup-trace-id: ${{ steps.setup.outputs.trace-id }} | |||
| stale_lock_file_failed: ${{ steps.check-lock-file.outputs.stale_lock_file_failed == 'true' }} | |||
| steps: | |||
| - name: Checkout actions folder | |||
| uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 | |||
| with: | |||
| repository: github/gh-aw | |||
| sparse-checkout: | | |||
| actions | |||
| persist-credentials: false | |||
| - name: Setup Scripts | |||
| id: setup | |||
| uses: github/gh-aw-actions/setup@v0.77.5 | |||
| uses: ./actions/setup | |||
| with: | |||
| destination: ${{ runner.temp }}/gh-aw/actions | |||
| job-name: ${{ github.job }} | |||
| safe-output-artifact-client: ${{ env.GH_AW_MAX_DAILY_AI_CREDITS != '' }} | |||
| env: | |||
| }, | ||
| "body": { | ||
| "type": "string", | ||
| "minLength": 20, | ||
| "description": "Detailed issue description in Markdown. Must be the final intended body \u2014 not a placeholder or test value. Do NOT repeat the title as a heading since it already appears as the issue's h1. Include context, reproduction steps, or acceptance criteria as appropriate." | ||
| }, |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. Testing noop |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38114 does not have the 'implementation' label (has_implementation_label=false) and has only 5 new lines in business logic directories, which is below the 100-line threshold (requires_adr_by_default_volume=false). No custom .design-gate.yml config present. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — no blocking issues; three non-blocking observations surfaced.
📋 Key Themes & Highlights
Key Themes
- Test boundary coverage: The new
create_issueminLength test block covers== 20(accept) and a short string (reject), but is missing the== 19(reject) case that fully pins boundary semantics. Also missing a whitespace-only body test that already exists forcreate_discussion. - Schema inconsistency follow-up: This PR establishes the correct pattern — Go config constant + JSON schema
minLengthaligned.create_discussionhas the same gap (Go: 64, JSON schema: none) and would benefit from the same fix.
Positive Highlights
- ✅ Named constant
MinIssueBodyLength = 20follows the existingMinDiscussionBodyLengthpattern exactly — easy to audit and change in one place - ✅ Both runtime and compiler-embedded JSON schemas updated together — the dual-write is easy to miss and was handled correctly
- ✅ Three new boundary tests (< 20 reject, > 20 accept, == 20 accept) and the
expect(body.length).toBe(20)assertion are a solid specification pattern - ✅ Existing test fixtures updated surgically to stay valid under the new constraint
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 537.8 AIC · ⌖ 13.5 AIC
| const result = validateItem({ type: "create_issue", title: "Test Issue", body }, "create_issue", 1); | ||
|
|
||
| expect(result.isValid).toBe(true); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The boundary test confirms == 20 is accepted, but has no companion test for == 19 (minLength − 1). Without it we cannot tell whether the implementation uses < 20 or <= 20 — a classic off-by-one gap.
💡 Suggested test at minLength − 1
it("should reject create_issue body at exactly minLength - 1", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
const body = "Exactly nineteen ch"; // 19 chars
expect(body.length).toBe(19);
const result = validateItem(
{ type: "create_issue", title: "Test Issue", body },
"create_issue",
1,
);
expect(result.isValid).toBe(false);
expect(result.error).toContain("too short");
});Pairing == 19 (reject) with == 20 (accept) fully pins the boundary semantics.
| const result = validateItem({ type: "create_issue", title: "Test Issue", body }, "create_issue", 1); | ||
|
|
||
| expect(result.isValid).toBe(true); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The create_discussion test suite covers the whitespace-trim edge case. The new create_issue block skips it. The validator trims before comparing (finalValue.trim().length < validation.minLength), so this behaviour is worth pinning explicitly for parity.
💡 Suggested whitespace test
it('should reject create_issue body that is only whitespace', async () => {
const { validateItem } = await import('./safe_output_type_validator.cjs');
// 25 spaces: raw length passes 20 but trimmed length is 0
const result = validateItem(
{ type: 'create_issue', title: 'Test Issue', body: ' ' },
'create_issue',
1,
);
expect(result.isValid).toBe(false);
expect(result.error).toContain('too short');
});Mirrors the existing create_discussion whitespace test and guards the trim-before-check contract.
| }, | ||
| "body": { | ||
| "type": "string", | ||
| "minLength": 20, |
There was a problem hiding this comment.
[/zoom-out] This PR correctly aligns the create_issue JSON schema with its Go validation config. However, create_discussion has MinDiscussionBodyLength = 64 in safe_outputs_validation_config.go (line 48) but no corresponding "minLength": 64 in either safe_outputs_tools.json. The gap means:
- Go sample validation (which compiles the JSON schema) will not catch short discussion bodies at compile time
- The LLM tool description gives no hint about the length floor
A follow-up to add "minLength": 64 on create_discussion.body would close the same class of inconsistency this PR is fixing for create_issue.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Test Inflation Note
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 3 new tests verify observable validation behavior (rejection, acceptance, and boundary acceptance for the new minLength constraint).
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — one regression + two tests now verifying the wrong thing
One critical regression and two test-correctness issues need fixing before merge.
### Blocking issues
1. (Critical) sampleRuntimeExpressionPlaceholder breaks compile-time validation for runtime-expression bodies — see inline comment on safe_outputs_tools.json:15
Adding "minLength": 20 to the JSON schema makes it impossible to use a ${{ ... }} expression as a sample body for create_issue. The placeholder substituted during validateSafeOutputsSamples is "aw_sample" (9 chars), which fails the new schema constraint. Every existing and future workflow with body: "${{ github.event.inputs.description }}" in a create-issue sample will be rejected at compile time. create_discussion deliberately omits minLength from its JSON schema for this reason.
2. (High) Two unchanged tests now test the wrong failure mode — see inline comment on samples_validation_test.go:22
TestValidateSafeOutputsSamples_MissingRequired: body"Body without title"is 18 chars, now fails onminLengthnot missing-title. Test passes but no longer tests what it claims.TestValidateSafeOutputsSamples_NonExpressionErrorsStillReported: body is a runtime expression → substituted to"aw_sample"(9 chars < 20) → fails onminLengthnot missing-title. Same problem.
3. (Medium) Missing TestCreateIssueBodyMinLength parity test — see inline comment on safe_outputs_validation_config.go:47
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.2 AIC
| }, | ||
| "body": { | ||
| "type": "string", | ||
| "minLength": 20, |
There was a problem hiding this comment.
Regression: sampleRuntimeExpressionPlaceholder ("aw_sample", 9 chars) is shorter than this new minLength: 20, breaking compile-time validation for create_issue samples whose body is a ${{ ... }} runtime expression.
💡 Details and fix options
validateSafeOutputsSamples in samples_validation.go substitutes runtime expressions with a placeholder before schema validation. For plain string fields with no pattern/format, placeholderForType unconditionally returns "aw_sample" (9 chars). Now that the JSON Schema requires body.length >= 20, "aw_sample" fails that check, and any workflow like:
samples:
- title: "Bug from ${{ github.event.inputs.component }}"
body: "${{ github.event.inputs.description }}"will be rejected at gh aw compile time with a spurious too
| { | ||
| "title": "Sample issue", | ||
| "body": "Sample body", | ||
| "body": "Sample issue body with enough detail.", |
There was a problem hiding this comment.
Two other tests in this file were not updated and now silently test the wrong failure path.
💡 Details
1. TestValidateSafeOutputsSamples_MissingRequired (line 56)
"body": "Body without title", // 18 chars — below the new minLength: 20The test's intent is to verify that a missing title surfaces an error. After this PR, the JSON Schema also requires body.length >= 20, so the 18-char body now triggers a minLength failure before the missing-title check. The test still passes (there is still an error), but it no longer verifies what its comment claims. Fix: update the body to ≥ 20 chars, e.g., "Body text without title.".
2. TestValidateSafeOutputsSamples_NonExpressionErrorsStillReported (line 253)
"body": "${{ github.event.inputs.body }}", // substituted → "aw_sample" (9 chars < 20)This test verifies that a runtime expression on body does not mask a missing-title error. After this PR, the substituted placeholder "aw_sample" (9 chars) fails the new minLength: 20 JSON Schema constraint, so the error that surfaces is now body-too-short, not missing-title. The test still passes (error is still non-nil with the right key/index prefix), but the stated invariant is no longer being tested. This is also a symptom of the regression flagged on safe_outputs_tools.json line 15.
| MaxBodyLength = 65000 | ||
| MaxGitHubUsernameLength = 39 | ||
| MaxGitHubTeamSlugLength = 100 | ||
| MinIssueBodyLength = 20 // Minimum body length for create_issue to prevent placeholder-only submissions |
There was a problem hiding this comment.
Missing parity test: no TestCreateIssueBodyMinLength analogous to the existing TestCreateDiscussionBodyMinLength.
💡 Details
safe_output_validation_config_test.go has TestCreateDiscussionBodyMinLength which asserts:
if bodyField.MinLength != MinDiscussionBodyLength { ... }No equivalent test was added for create_issue. A future refactor that accidentally removes MinLength from the create_issue body field would be silently undetected. Add:
func TestCreateIssueBodyMinLength(t *testing.T) {
config, ok := ValidationConfig["create_issue"]
require.True(t, ok)
body, ok := config.Fields["body"]
require.True(t, ok)
if body.MinLength != MinIssueBodyLength {
t.Errorf("create_issue body MinLength = %d, want %d", body.MinLength, MinIssueBodyLength)
}
}Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher pass and pushed fixes in |
|
@copilot review comments from GitHub-actions bot |
Reviewed the GitHub-actions bot feedback on this PR. The actionable items it raised are already addressed in |
|
@copilot review all comments and address unresolved review feedback.
|
The daily documentation report issue thread identified that low-signal/placeholder issue bodies were getting through safe outputs, which contributed to empty or non-actionable generated content. This change enforces a stricter floor for
create_issuebody content by requiring at least 20 characters.Validation source of truth
MinIssueBodyLength = 20inpkg/workflow/safe_outputs_validation_config.go.MinLengthtocreate_issue.bodyin the safe-output validation config map.pkg/workflow/safe_output_validation_config_test.goto assertcreate_issue.body.MinLength == MinIssueBodyLength.Tool schema alignment (runtime + compiler copies)
"minLength": 20tocreate_issue.inputSchema.properties.bodyin:actions/setup/js/safe_outputs_tools.jsonpkg/workflow/js/safe_outputs_tools.jsonRuntime-expression sample validation compatibility
pkg/workflow/samples_validation.goso schema-aware placeholder substitution for string runtime expressions honors JSON SchemaminLengthconstraints.${{ ... }}expressions are used increate_issue.bodysamples under the newminLengthrule.Targeted test updates
safe_output_type_validator.test.cjswith explicit coverage for:< 20characters19characters (boundary - 1)> 20characters20characters (boundary)pkg/workflow/samples_validation_test.goso missing-required-field scenarios still validate the intended error path under the new constraint.Scope cleanup
create_issuebody min-length enforcement and its directly coupled validation/test behavior.