fix: enforce minLength on update_release body to block placeholder submissions#39713
Conversation
…ssions
Investigation of run 27655450946 found that the agent emitted a
placeholder {"body":"test",...} update_release output first, then the
real release notes second. Since max:1, the "test" body was applied
and the real content rejected.
Root cause: update_release had no MinLength on the body field, unlike
create_issue (MinLength:20) and create_discussion (MinLength:64).
Fix:
- Add MinReleaseBodyLength = 20 constant to safe_outputs_validation_config.go
- Apply MinLength: MinReleaseBodyLength to update_release body field
- Add minLength: 20 and improved description to update_release body in
both safe_outputs_tools.json files
- Add 3 tests covering the new minLength enforcement
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR tightens safe-output validation for the update_release tool to prevent short placeholder release bodies (e.g., "test") from being accepted when workflows only permit a single update_release emission.
Changes:
- Add
MinReleaseBodyLength = 20and enforce it viaMinLengthonupdate_release.bodyin the safe-outputs validation config. - Update both copies of
safe_outputs_tools.jsonto declareminLength: 20forupdate_release.bodyand clarify the “no placeholders” requirement in the description. - Extend the JS validator test suite to include
update_releaseand add minLength-focused test cases.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_validation_config.go | Enforces a minimum length for update_release.body via MinReleaseBodyLength. |
| pkg/workflow/js/safe_outputs_tools.json | Updates tool schema/docs to reflect update_release.body minLength: 20. |
| actions/setup/js/safe_outputs_tools.json | Mirrors the tool schema/docs update for the runtime copy. |
| actions/setup/js/safe_output_type_validator.test.cjs | Adds update_release to the sample validation config and adds minLength tests. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| it("should reject update_release body that is only whitespace below minLength", async () => { | ||
| const { validateItem } = await import("./safe_output_type_validator.cjs"); | ||
|
|
||
| const result = validateItem({ type: "update_release", tag: "v1.0.0", operation: "prepend", body: " test " }, "update_release", 1); | ||
|
|
| "update_release": { | ||
| DefaultMax: 1, | ||
| Fields: map[string]FieldValidation{ | ||
| "tag": {Type: "string", Sanitize: true, MaxLength: 256}, | ||
| "operation": {Required: true, Type: "string", Enum: []string{"replace", "append", "prepend"}}, | ||
| "body": {Required: true, Type: "string", Sanitize: true, MaxLength: MaxBodyLength}, | ||
| "body": {Required: true, Type: "string", Sanitize: true, MaxLength: MaxBodyLength, MinLength: MinReleaseBodyLength}, | ||
| }, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 0 ( Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — the fix is correct and addresses the production incident cleanly; comments are improvement suggestions only.
📋 Key Themes & Highlights
Key Themes
- Missing Go-level regression test:
TestCreateIssueBodyMinLengthandTestCreateDiscussionBodyMinLengthboth exist;TestUpdateReleaseBodyMinLengthdoes not. The fix correctly wires the constant, but the Go-level guard that would catch future accidental unwiring is absent. - Whitespace test name/content mismatch: The third JS test is labelled "only whitespace" but exercises a whitespace-padded string. A purely-whitespace body is not explicitly covered.
- Formatting noise in
pkg/workflow/js/safe_outputs_tools.json: +694/−211 lines for a one-field change. The two JSON copies are structurally identical (verified) but formatted differently, making future diffs hard to review.
Positive Highlights
- ✅ Root cause correctly identified and fixed with a targeted constant + field wiring — no over-engineering
- ✅ Parity with the existing
create_issue/create_discussionpattern: same threshold, same comment style - ✅ Three JS regression tests cover the most important cases (reject placeholder, accept real content)
- ✅ Updated
descriptionfield in the JSON schema clearly signals the "final intended content" expectation to the agent - ✅ Both
actions/setup/js/andpkg/workflow/js/schema copies correctly updated
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| expect(result.isValid).toBe(true); | ||
| }); | ||
|
|
||
| it("should reject update_release body that is only whitespace below minLength", async () => { |
There was a problem hiding this comment.
[/tdd] Misleading test name: the body value " test " is whitespace-padded text, not purely whitespace. An actual pure-whitespace string (e.g., " ") is a distinct edge case that is currently untested.
💡 Suggested additions
Rename the existing test to reflect what it actually covers:
it("should reject update_release body whose trimmed length is below minLength (whitespace-padded)", async () => {And add a dedicated purely-whitespace test:
it("should reject update_release body that is purely whitespace", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
const result = validateItem(
{ type: "update_release", tag: "v1.0.0", operation: "replace", body: " " },
"update_release", 1
);
expect(result.isValid).toBe(false);
expect(result.error).toContain("too short");
});This closes the gap between what the test name promises and what it actually verifies.
| expect(result.error).toContain("too short"); | ||
| }); | ||
|
|
||
| it("should reject update_release body shorter than minLength (e.g. 'test')", async () => { |
There was a problem hiding this comment.
[/tdd] All three new tests use operation: "prepend". Since minLength enforces a floor regardless of operation, adding one test with operation: "replace" (the most common case) would confirm the constraint is operation-agnostic and guard against any future per-operation branching in the validator.
💡 Suggested addition
it("should reject update_release body shorter than minLength for replace operation", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
const result = validateItem(
{ type: "update_release", tag: "v1.0.0", operation: "replace", body: "test" },
"update_release", 1
);
expect(result.isValid).toBe(false);
expect(result.error).toContain("too short");
});| "tag": {Type: "string", Sanitize: true, MaxLength: 256}, | ||
| "operation": {Required: true, Type: "string", Enum: []string{"replace", "append", "prepend"}}, | ||
| "body": {Required: true, Type: "string", Sanitize: true, MaxLength: MaxBodyLength}, | ||
| "body": {Required: true, Type: "string", Sanitize: true, MaxLength: MaxBodyLength, MinLength: MinReleaseBodyLength}, |
There was a problem hiding this comment.
[/tdd] The codebase has TestCreateIssueBodyMinLength and TestCreateDiscussionBodyMinLength in safe_output_validation_config_test.go (lines 300–330), but there is no corresponding TestUpdateReleaseBodyMinLength. Given this is a production incident fix, adding a Go-level regression test follows the established pattern and ensures the constant wiring is validated independently of the JS tests.
💡 Suggested test (add to `safe_output_validation_config_test.go`)
func TestUpdateReleaseBodyMinLength(t *testing.T) {
config, ok := ValidationConfig["update_release"]
if !ok {
t.Fatal("update_release not found in ValidationConfig")
}
bodyField, ok := config.Fields["body"]
if !ok {
t.Fatal("body field not found in update_release validation config")
}
if bodyField.MinLength != MinReleaseBodyLength {
t.Errorf("update_release body MinLength = %d, want %d", bodyField.MinLength, MinReleaseBodyLength)
}
}This mirrors the exact pattern used for create_issue and create_discussion and would have caught the original production gap earlier.
| "type": "string", | ||
| "description": "Release body content in Markdown. For 'replace', this becomes the entire release body. For 'append'/'prepend', this is added with a separator." | ||
| "description": "Release body content in Markdown. Must be the final intended content — not a placeholder or test value. For 'replace', this becomes the entire release body. For 'append'/'prepend', this is added with a separator.", | ||
| "minLength": 20 |
There was a problem hiding this comment.
[/zoom-out] The actual logic change here is a single "minLength": 20 field, but the diff shows +694/−211 lines due to JSON formatting differences between this file and actions/setup/js/safe_outputs_tools.json. Both files are structurally identical (confirmed), but the per-element array expansion makes the change nearly unreviable.
📋 Context and suggestion
If these two files are always kept in sync (same content, different formatter), consider:
- Canonicalising on one formatter — so future diffs are minimal and reviewable.
- Or generating
pkg/workflow/js/fromactions/setup/js/— a single source of truth would eliminate the risk of accidental drift and remove this class of noisy diffs.
As a minimum, the PR description could note that the bulk of this diff is formatting-only to help reviewers focus on line 1415.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer
| it("should reject update_release body that is only whitespace below minLength", async () => { | ||
| const { validateItem } = await import("./safe_output_type_validator.cjs"); | ||
|
|
||
| const result = validateItem({ type: "update_release", tag: "v1.0.0", operation: "prepend", body: " test " }, "update_release", 1); |
There was a problem hiding this comment.
Whitespace test uses wrong input — doesn't exercise the trim path at all.
The test name promises coverage of "only whitespace below minLength", but " test " (10 chars raw) fails because 10 < 20 — the trim call is irrelevant here. The test is a near-duplicate of the first test and doesn't add useful coverage.
💡 What to test instead
The dangerous case is content whose raw length meets the minLength threshold but whose trimmed length falls below it — e.g. 20+ spaces. The existing create_issue suite covers this correctly:
// create_issue's whitespace test — 25 spaces, raw length ≥ 20, trimmed → "" → rejected
validateItem({ ..., body: " " }, ...)The update_release version should mirror that:
it("should reject update_release body that is only whitespace even when raw length meets minLength", async () => {
const { validateItem } = await import("./safe_output_type_validator.cjs");
// 20 spaces: passes JSON Schema minLength:20, but trim() → "" → runtime should reject it
const result = validateItem(
{ type: "update_release", tag: "v1.0.0", operation: "prepend", body: " " },
"update_release", 1
);
expect(result.isValid).toBe(false);
expect(result.error).toContain("too short");
});Without this, removing the .trim() call on line 401 of the validator would silently pass all existing update_release tests while breaking the intended protection.
In run 27655450946, the agent emitted a placeholder
{"body":"test",...}as its firstupdate_releaseoutput, followed by the real release notes. Withmax:1, the placeholder was applied and the real content rejected — prepending the word "test" to the v0.80.1 release.The gap:
update_release.bodyhad noMinLength, unlikecreate_issue(20) andcreate_discussion(64), so any non-empty string passed validation.Changes
safe_outputs_validation_config.go— AddMinReleaseBodyLength = 20; applyMinLength: MinReleaseBodyLengthtoupdate_release.bodysafe_outputs_tools.json(bothpkg/andactions/setup/js/) — AddminLength: 20toupdate_release.body; update description to say "Must be the final intended content — not a placeholder or test value"safe_output_type_validator.test.cjs— Addupdate_releaseto sample validation config; add 3 minLength tests (reject"test", reject whitespace-only, accept real content)With this fix,
{"body":"test",...}produces: