[purelock] Lock down extractAmbientFolders, secretRequirementsFromAuthDefinition, firstForbiddenCharInModelToken with pure-function test su [Content truncated due to length] - #52719
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hey Assessment✅ On-topic — Aligns with the project's testing and coverage priorities Contribution Process NoteThe gh-aw CONTRIBUTING.md restricts non-core team members from opening PRs directly. This PR is authored by What looks great:
This PR is ready for review by the core team.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines.
Why this stays non-blocking
The patch only adds tests, and the added cases are aligned with the current function behavior in the touched code paths. I did not find a correctness, security, or performance regression introduced by these test-only changes. The background sub-agent produced no usable findings.🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.38 AIC · ⌖ 7.53 AIC · ⊞ 6.5K
Comment /review to run again
There was a problem hiding this comment.
The PR adds pure-function test suites for extractAmbientFolders, secretRequirementsFromAuthDefinition, and firstForbiddenCharInModelToken. Tests are well-structured with table-driven cases, proper parallel execution, correct loop-variable capture (tt := tt), and a fuzz test with sound invariant checks. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.9 AIC · ⌖ 7.87 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #52719 · auto · 23.3 AIC · ⌖ 2.89 AIC · ⊞ 6.8K
Comment /ponytail to run again
| engineName string | ||
| want []SecretRequirement | ||
| }{ | ||
| { |
There was a problem hiding this comment.
L830: delete: tt := tt loop-var-capture shim. go.mod pins go 1.26.5 (post-1.22), loop vars are per-iteration; drop the line.
| } | ||
|
|
||
| for _, tt := range tests { | ||
| tt := tt |
There was a problem hiding this comment.
L77: delete: tt := tt loop-var-capture shim. go.mod pins go 1.26.5 (post-1.22), loop vars are per-iteration; drop the line.
| } | ||
|
|
||
| for _, tt := range tests { | ||
| tt := tt |
There was a problem hiding this comment.
L35: delete: tt := tt loop-var-capture shim. go.mod pins go 1.26.5 (post-1.22), loop vars are per-iteration; drop the line.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one minor gap in the fuzz invariant; no blocking issues. See inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.9 AIC · ⌖ 7.18 AIC · ⊞ 7.1K
Comment /matt to run again
🧪 Test Quality Sentinel Report❌ Test Quality Score: 100/100 — Excellent (blocked by violations)
📊 Metrics (3 tests + 1 fuzz)
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 100/100 but blocked by 2 hard violations. 2 new Go test files (ambient_folders_test.go, first_forbidden_char_in_model_token_test.go) are missing the mandatory (go/redacted):build !integration tag on line 1. Review flagged tests in the comment above.
✅ Design Decision Gate — ADR VerifiedADR reviewed: ADR-52295: Targeted Pure-Function Test Coverage via PureLock — implementation aligns with the stated decision. Great work! 🏗️ 📋 Verification SummaryThis PR is a direct output of the PureLock workflow described in the ADR. The implementation matches the stated decision on all key points:
Minor scope note (not a divergence): The ADR's Decision section mentions merging tests into
|
There was a problem hiding this comment.
Pull request overview
Adds focused tests for three pure helper functions in workflow parsing and CLI secret handling.
Changes:
- Tests ambient-folder extraction and authentication secret requirements.
- Adds table and fuzz tests for forbidden model-token characters.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/ambient_folders_test.go |
Tests supported inputs and errors. |
pkg/cli/engine_secrets_test.go |
Tests authentication secret mappings. |
pkg/workflow/first_forbidden_char_in_model_token_test.go |
Tests and fuzzes token-character validation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
pkg/workflow/first_forbidden_char_in_model_token_test.go:75
- The nonzero-result invariant only proves that the returned rune occurs somewhere in the input. It therefore accepts an allowed rune or a later forbidden rune, so it does not lock down the function's defining “first forbidden character” behavior. Scan to the first forbidden rune and compare it directly with
got.
// A forbidden rune must actually be present in s.
found := false
for _, r := range s {
if r == got {
found = true
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| func FuzzFirstForbiddenCharInModelToken(f *testing.F) { | ||
| seeds := []string{ | ||
| "", |
Adds maximum-coverage testify test suites for three pure functions selected by the PureLock precompute ranking (top 3 by score, no prior processing in cache memory).
extractAmbientFolders(pkg/workflow/ambient_folders.go:38)func extractAmbientFolders(frontmatter map[string]any) ([]string, error)Purity: no observable side effects — reads only its
frontmatterargument, performs type assertions and builds/returns a new slice; no I/O, no globals, no mutation of input.pkg/workflowoverall → 87.2%.map[string]any).secretRequirementsFromAuthDefinition(pkg/cli/engine_secrets.go:136)func secretRequirementsFromAuthDefinition(auth *workflow.AuthDefinition, engineName string) []SecretRequirementPurity: no observable side effects — builds and returns a
[]SecretRequirementpurely from its inputs; no I/O or global state.has_test_filewas true but this specific function was untested (0% coverage). Added a newTestSecretRequirementsFromAuthDefinitionto the existingengine_secrets_test.go.pkg/cli→ 61.9% overall (baseline unaffected functions unchanged).assert.Equalon full[]SecretRequirementslices), covering OAuth (both/only-id/only-secret/neither refs), api-key, bearer, unset-strategy-default, and empty-secret default branches.firstForbiddenCharInModelToken(pkg/workflow/model_identifier.go:122)func firstForbiddenCharInModelToken(s string) runePurity: no observable side effects — pure string-scanning function with no I/O or global state.
pkg/workflow→ 87.2% overall.TestFirstForbiddenCharInModelToken) + 1 fuzz target (FuzzFirstForbiddenCharInModelToken), 14 subtests, 14 assertions, plus fuzz invariant checks (no false positives/negatives on forbidden-char detection).fuzz_friendly: true), seeded with allowed/forbidden/unicode/emoji cases; asserts the returned rune is either 0 (all chars allowed) or an actual forbidden rune present in the input.Validation performed
gofmt -l— clean on all three test files.go vet ./pkg/workflow/... ./pkg/cli/...— clean.go test ./pkg/workflow/ -race -count=1— pass.go test ./pkg/cli/ -race -count=1 -run TestSecretRequirementsFromAuthDefinition— pass (two pre-existing, unrelated failures inpkg/cli—TestRenderScheduleCalendarCell_UsesANSIInColorTerminalandTestConfirmRunAddedWorkflow_ContextCancelled— are environment-dependent, e.g. missingghauth/color-terminal detection, and are unaffected by this change).Only
*_test.gofiles were touched; no production code was modified.