Formalize CTR-022/CTR-023 behavior with predicate-level unit coverage - #51820
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds predicate-level unit coverage for CTR-022 git argument validation and CTR-023 bash restriction detection.
Changes:
- Tests git ref/path validation boundaries and errors.
- Tests bash restriction semantics, including mixed wildcards.
- A path NUL-byte case remains uncovered.
Show a summary per file
| File | Description |
|---|---|
pkg/gitutil/gitutil_ctr_formal_test.go |
Adds CTR-022 validation tests. |
pkg/workflow/agent_validation_formal_test.go |
Adds CTR-023 predicate tests. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| assert.ErrorContains(t, err, "must not be empty") | ||
| } | ||
|
|
||
| func TestValidateGitPath_SafePathAccepted(t *testing.T) { |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: PR only adds two new test files (gitutil_ctr_formal_test.go, agent_validation_formal_test.go) with straightforward table-driven/individual test cases for existing exported functions. No production code changes, no new abstractions, no speculative flexibility, no reinvented libraries. Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
![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 — requesting changes on two coverage gaps.
📋 Key Themes & Highlights
Issues
- NUL byte gap in
ValidateGitPath: CTR-022 requires NUL rejection for both refs and paths; only refs are tested. - Missing actionability contract for paths:
ValidateGitRefverifies rejected values appear in error messages;ValidateGitPathdoes not. - Multi-assertion functions: Several
HasBashExplicitRestrictiontests pack multiple cases into one function, making failure output harder to diagnose.
Positive Highlights
- ✅ Excellent predicate-to-behavior mapping in
HasBashExplicitRestrictiontests — the mixed-wildcard edge case is exactly the kind of implementation-specific corner case formal tests should pin. - ✅ Consistent use of
require.Error→assert.ErrorContainspattern in gitutil tests is clean and readable. - ✅ Good build-tag hygiene (
(go/redacted):build !integration).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.5 AIC · ⌖ 7.76 AIC · ⊞ 7.1K
Comment /matt to run again
| assert.ErrorContains(t, err, "must not be empty") | ||
| } | ||
|
|
||
| func TestValidateGitRef_ErrorMessagesAreActionable(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] TestValidateGitRef_ErrorMessagesAreActionable has no ValidateGitPath counterpart — path error messages are never verified to include the offending value.
💡 Suggested addition
Add a parallel TestValidateGitPath_ErrorMessagesAreActionable covering the three rejected path variants:
func TestValidateGitPath_ErrorMessagesAreActionable(t *testing.T) {
invalidPaths := []string{
"-evil",
"/etc/passwd",
"dir/../../etc/passwd",
}
for _, p := range invalidPaths {
t.Run(p, func(t *testing.T) {
err := ValidateGitPath(p)
require.Error(t, err)
assert.ErrorContains(t, err, fmt.Sprintf("%q", p))
})
}
}Without this, a refactor silencing path values in error messages goes undetected.
@copilot please address this.
| assert.ErrorContains(t, err, "must not be empty") | ||
| } | ||
|
|
||
| func TestValidateGitPath_SafePathAccepted(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] NUL byte injection is a CTR-022 concern for both refs and paths, but ValidateGitPath has no NUL test — only refs are covered (line 36).
💡 Suggested test
func TestValidateGitPath_NulByteRejected(t *testing.T) {
p := "file\x00evil"
err := ValidateGitPath(p)
require.Error(t, err)
assert.ErrorContains(t, err, "must not contain NUL bytes")
assert.ErrorContains(t, err, fmt.Sprintf("%q", p))
}If ValidateGitPath does not currently reject NUL bytes, this test will go red and surface the gap.
@copilot please address this.
| ) | ||
|
|
||
| func TestHasBashExplicitRestriction_WildcardIsSafe(t *testing.T) { | ||
| assert.False(t, HasBashExplicitRestriction(map[string]any{"bash": []any{"*"}})) |
There was a problem hiding this comment.
[/tdd] Table-driven test functions use a single assertion each — grouping multiple assert.False / assert.True calls in one function obscures which case failed.
💡 Suggested refactor
Use subtests for multi-case functions so failure output names the failing input:
func TestHasBashExplicitRestriction_WildcardIsSafe(t *testing.T) {
cases := []struct{ name string; val any }{
{"star", []any{"*"}},
{"colon-star", []any{":*"}},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
assert.False(t, HasBashExplicitRestriction(map[string]any{"bash": tc.val}))
})
}
}This is low-cost now and pays off as the wildcard rule evolves.
@copilot please address this.
There was a problem hiding this comment.
Review: Formal CTR-022/CTR-023 test coverage
The new predicate-level tests are a good addition and correctly exercise the key security boundaries. Two issues worth addressing:
🔴 Missing: ValidateGitPath NUL byte check (CTR-022 gap)
As flagged in the existing inline comment: ValidateGitPath has no NUL byte guard in production code, and no NUL test in the new suite. A ValidateGitPath("file\x00evil") call today returns nil — passes silently. CTR-022 requires NUL rejection for paths too. Fix: add strings.ContainsRune(path, '\x00') to ValidateGitPath and a matching test case.
🟡 TestValidateGitRef_ErrorMessagesAreActionable is redundant
Lines 102–115 re-test the same three invalid refs (-evil, main\x00evil, main..evil) checking only that fmt.Sprintf("%q", ref) appears in the error. The three individual reject tests (HyphenPrefixRejected, NulByteRejected, TraversalRejected) already include this assert.ErrorContains check. The omnibus test adds no new signal and should be removed to reduce maintenance overhead.
✅ agent_validation_formal_test.go looks good
All HasBashExplicitRestriction edge cases are well-covered and correctly match the production logic in agent_validation.go.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.8 AIC · ⌖ 6.9 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (17 tests)
|
| Component | Calculation | Points |
|---|---|---|
| Design ratio | 17/17 × 40 | 40 |
| Edge/error coverage | 14/17 × 30 | ~25 |
| Duplicate penalty | 0 clusters × 5 | 0 deducted (max 20) |
| Inflation bonus | No inflation | +10 |
| Total | 95/100 |
No production files changed in this PR — test:prod inflation ratio is not applicable (tests cover pre-existing
ValidateGitRef/ValidateGitPath/HasBashExplicitRestrictionfunctions).
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). All 17 tests enforce security-critical input-validation invariants for CTR-022 (git ref injection prevention) and CTR-023 (bash-restriction predicate correctness). No violations detected.
🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 38.6 AIC · ⌖ 7.34 AIC · ⊞ 7.6K · ◷
Comment /review to run again
…ests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🎉 This pull request is included in a new release. Release: |
This issue formalizes two threat-detection rules introduced in v1.0.20—CTR-022 (git subprocess argument injection) and CTR-023 (bash allowlist illusion)—against exported implementations. This PR adds explicit boundary/edge-case tests that map directly to the formal predicates and implementation-specific corner cases.
CTR-022:
gitutilcontract coverage (pkg/gitutil/gitutil_ctr_formal_test.go)ValidateGitRefandValidateGitPathacceptance/rejection boundaries:..)CTR-023: bash restriction semantics (
pkg/workflow/agent_validation_formal_test.go)HasBashExplicitRestrictionsemantics:["*"],[":*"]) treated as unrestricted/safetrue/niltreated as unrestricted/safefalse, empty list, and named non-wildcard lists treated as explicit restrictionExample predicate-to-behavior mapping