[WIP] Formalize security and error handling for replace-label spec - #51129
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7deb91e6-9c90-4f01-8cdd-248ad3f2f8d7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage ResultCategory: test · Risk: low · Priority: low (score 28/100 — impact 12, urgency 8, quality 8) Notes: Draft, checklist-style WIP adding formal security tests for replace-label spec (P1–P8 predicates). No CI runs yet. Revisit once checklist items are completed and PR is marked ready.
|
There was a problem hiding this comment.
Pull request overview
Adds formal security and error-handling models for the replace-label safe-output specification.
Changes:
- Adds P1–P8 formal predicates.
- Covers sanitization, retries, REST failures, label constraints, enforcement, and token scope.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/replace_label_security_formal_test.go |
Adds formal security and error-handling tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/replace_label_security_formal_test.go:36
- This does not match the referenced standard safe-output sanitizer.
sanitizeLabelContentstrips complete ANSI sequences and applies Unicode hardening, while this loop removes only ESC and leaves strings such as[31mbug[0m; it also differs on embedded tabs/newlines. P1 therefore formalizes behavior that conflicts with RL-007. Please exercise the canonical sanitizer (for example through a JS harness) or mirror its full contract and vectors rather than defining a narrower sanitizer here.
// C0 controls (including \t, \n, \r), DEL and C1 controls are removed.
if r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) {
continue
}
b.WriteRune(r)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| // As with the sibling formal files, the helpers below are spec-level models of | ||
| // the semantics described in the specification, not wrappers around the | ||
| // production JavaScript handler (actions/setup/js/replace_label.cjs). | ||
| // Regressions in the handler itself are detected by the JavaScript test suite. |
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/replace_label_security_formal_test.gowith P1–P8 formal predicates (sanitization, rate-limit retry, REST label array, hard errors, server-side enforcement, token scope)go test ./pkg/workflow/... -run TestFormalSecmake fmt/ validation