[purelock] Lock down selectHistoricalOperationalValueGrader, extractHostFromRemoteURL, extractOTLPAttributesFromObsMap with pure [Content t - #56895
Conversation
…, extractHostFromRemoteURL, extractOTLPAttributesFromObsMap - pkg/cli/graders_operational_value_regrade.go: selectHistoricalOperationalValueGrader (coverage 0% -> 100%) - pkg/cli/git.go: extractHostFromRemoteURL (coverage 64% -> 96%) - pkg/parser/import_observability.go: extractOTLPAttributesFromObsMap (coverage 42.1% -> 100%) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: non-blocking
This PR only adds targeted tests, and I did not find an actionable correctness, performance, or maintainability problem in the changed lines.
Notes
pkg/cli/git_test.goadds coverage for malformed URL fallback branches and the scp-like non-match case.pkg/cli/graders_operational_value_regrade_test.goexercises the expected nil/duplicate/mismatch error paths and the success path for the historical grader selector.pkg/parser/import_observability_test.gocovers absent, malformed, empty, and mixed-type observability attribute inputs.- The requested
grumpy-codersub-agent could not be used because the executable is unavailable in this environment, so its output was discarded.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 4.18 AIC · ⌖ 7.33 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — no blocking issues. Three minor observations posted as inline comments.
📋 Key Themes & Highlights
Key Themes
- nil vs empty map asymmetry (
import_observability_test.goline 54):extractOTLPAttributesFromObsMapreturnsmap[string]string{}for empty attributes butnilfor all absent-key paths — worth documenting whether that distinction is intentional. - Hardcoded result ID (
graders_operational_value_regrade_test.goline 355): success-path assertion uses a literal"operational-value"instead of a table field, reducing extensibility. - Undocumented fallback behaviour (
git_test.goline 591): the"%zz"expected value is surprising without a comment explaining the manual-fallback logic.
Positive Highlights
- ✅ Comprehensive table-driven tests across all 3 functions with clear names
- ✅ Consistent
t.Parallel()usage; no test-isolation issues - ✅ Error assertions check both the error message and that pointers are nil — good defensive style
- ✅
import_observability_test.gousestestify/assertconsistently with the rest ofpkg/parser
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 25.6 AIC · ⌖ 10.7 AIC · ⊞ 7.7K
Comment /matt to run again
| name: "empty attributes map returns empty result", | ||
| obs: map[string]any{"otlp": map[string]any{"attributes": map[string]any{}}}, | ||
| expected: map[string]string{}, | ||
| }, |
There was a problem hiding this comment.
[/tdd] The "empty attributes map" case returns map[string]string{} while all "not found" paths return nil — verify this asymmetry is intentional in the production code.
💡 Why this matters
Callers that guard with if result != nil will see a non-nil result for an empty attributes map, even though there is nothing useful in it. If the production function intends nil to mean "no OTLP config" uniformly, this test case may be revealing an inconsistency worth fixing.
Consider adding a comment in the test (or production code) explaining why empty-attributes is distinct from absent-attributes.
@copilot please address this.
| if tt.wantErr != "" { | ||
| if err == nil || !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("expected error containing %q, got %v", tt.wantErr, err) | ||
| } |
There was a problem hiding this comment.
[/tdd] The success-path assertion hardcodes "operational-value" as the expected result.ID rather than reading it from tt — this means adding a second success case with a different ID would silently pass a wrong assertion.
💡 Suggested fix
Add a wantResultID field to the test struct (mirroring wantEntryID) and assert against it:
if result == nil || result.ID != tt.wantResultID {
t.Fatalf("expected result with ID %q, got %v", tt.wantResultID, result)
}For the existing success case, set wantResultID: "operational-value".
@copilot please address this.
| }, | ||
| { | ||
| name: "SSH scp-like with slash before colon does not match and defaults to github.com", | ||
| url: "some/path:notaport", |
There was a problem hiding this comment.
[/tdd] The case "malformed URL falls back to manual parsing with no userinfo and no path" expects "%zz" as the host — this is an edge case where neither @ nor / is present after the scheme, and the test documents that the raw remainder is returned verbatim.
Consider adding a brief comment to this test case (or the production code) explaining that "%zz" is returned because the manual-fallback strips the scheme and finds no delimiter, so the residual string IS the "host". Without that comment, a future reader might assume it is a bug.
@copilot please address this.
ADR RequiredThis PR triggered the design decision gate because it adds more than 100 new lines in business-logic directories ( I did not find an existing ADR in the PR body, and the latest ADR already on the branch ( To keep the branch mergeable, I added a draft ADR here:
Evidence used:
Next actionPlease review and refine the draft ADR so the stated context, decision, trade-offs, and consequences match the intent of this PR before merge.
|
There was a problem hiding this comment.
Tests-only PR adding pure-lock coverage for selectHistoricalOperationalValueGrader, extractHostFromRemoteURL, and extractOTLPAttributesFromObsMap. Edge cases are well-chosen and thorough. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 20.2 AIC · ⌖ 10.5 AIC · ⊞ 6.3K
There was a problem hiding this comment.
The new coverage adds a lot of table-driven boilerplate for a tiny helper; trimming the matrix to one success case and one representative error case would preserve the signal with less test code.
net: -80 lines possible.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
ab.chatgpt.comapi.github.comchatgpt.comgithub.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #56895 · codex · mai10 · 7.1 AIC · ⌖ 0.64 AIC · ⊞ 13.5K
Comment /ponytail to run again
| } | ||
| } | ||
|
|
||
| func TestSelectHistoricalOperationalValueGrader(t *testing.T) { |
There was a problem hiding this comment.
L255: shrink: 10-case table for a tiny pure helper. Keep one success case plus one representative error case.
There was a problem hiding this comment.
🟡 Changes recommended
Some expectations contradict documented contracts, and pointer identity remains unverified.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds targeted tests to lock down three pure parsing and selection helpers.
Changes:
- Covers operational-value grader selection and errors.
- Expands malformed Git remote URL cases.
- Adds OTLP attribute extraction tests.
File summaries
| File | Description |
|---|---|
pkg/cli/graders_operational_value_regrade_test.go |
Tests historical grader selection. |
pkg/cli/git_test.go |
Tests remote-host parsing fallbacks. |
pkg/parser/import_observability_test.go |
Tests OTLP attribute extraction. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if entry == nil || entry.ID != tt.wantEntryID { | ||
| t.Fatalf("expected entry with ID %q, got %v", tt.wantEntryID, entry) | ||
| } | ||
| if result == nil || result.ID != "operational-value" { | ||
| t.Fatalf("expected result with ID operational-value, got %v", result) | ||
| } |
| name: "empty attributes map returns empty result", | ||
| obs: map[string]any{"otlp": map[string]any{"attributes": map[string]any{}}}, | ||
| expected: map[string]string{}, |
| name: "malformed URL falls back to manual parsing with no userinfo and no path", | ||
| url: "https://%zz", | ||
| expected: "%zz", |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in The Removed both redundant Validation: |
|
🎉 This pull request is included in a new release. Release: |
PureLock 🔐 — 3 pure functions locked down
Precomputed coverage source:
func-coverage.txt(merged coverage profile + fixed-point purity analysis).1.
selectHistoricalOperationalValueGraderpkg/cli/graders_operational_value_regrade.go:296func selectHistoricalOperationalValueGrader(manifest *operationalValueGraderManifest, artifact *graderResultsArtifact, runID string) (*operationalValueGraderManifestEntry, *graderArtifactResult, error)purity_notessaid "no observable side effects detected." Verified by inspection: the function only reads fields off itsmanifest/artifactpointer arguments, performs no I/O, no goroutines, no global/package state mutation, and returns derived pointers/errors deterministically based solely on its inputs.pkg/cli63.9% → 64.0%.2.
extractHostFromRemoteURLpkg/cli/git.go:141func extractHostFromRemoteURL(remoteURL string) stringpurity_notessaid "no observable side effects detected." Verified: pure string/URL parsing vianet/url.Parseandstringshelpers, no I/O, no shared state, deterministic output for a given input string.pkg/cli63.9% → 64.0%.TestExtractHostFromRemoteURLtable with 4 new cases exercising theurl.Parsefailure fallback branches (malformed%zzescapes with/without userinfo and path) and the scp-like "slash before colon" guard that falls through to thegithub.comdefault.fuzz_friendlywas true in the candidate metadata; a dedicated fuzz test was not added since the manual fallback branches are now exercised directly by the new table cases and residual risk is low. Residual uncovered: one defensive branch inside the manual-fallback path for URLs with neither@nor/after the scheme, considered low-value to special-case further.3.
extractOTLPAttributesFromObsMappkg/parser/import_observability.go:173func extractOTLPAttributesFromObsMap(obs map[string]any) map[string]stringpurity_notessaid "no observable side effects detected." Verified: pure map traversal/type-assertion logic building and returning a new map, no I/O, no mutation of the input, no global state.pkg/parser73.7% → 73.9%.pkg/parser/import_observability_test.gowith 1 table-driven test, 9 subtests, 9 assertions covering nil map, empty map, missingotlpkey, non-mapotlp/attributesvalues, empty attributes, string-valued attributes, non-string values being silently dropped (int/bool/nested map/slice/nil), and empty-key filtering.Validation performed
gofmt -lclean on all 3 modified/added test files.go vet ./pkg/cli/ ./pkg/parser/passes with no findings.go test ./pkg/cli/ -race -count=1andgo test ./pkg/parser/ -race -count=1both pass for the new/targeted tests (full-package runs surface a handful of pre-existing, unrelated failures caused by this sandbox's read-only/tmpfilesystem and lack of outbound network/GH auth — e.g.TestValidateWithSchemaAndLocation_CleanedErrorMessage,TestBuildLogsFileResponse_*,TestInstallCopilotCLIScript*— none of which touch the 3 functions locked down here).go tool cover -funcon isolated before/after profiles per package.pkg/cli+pkg/parsergenerated and uploaded aspurelock/generated-tests.Warning
Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
api.github.comgithub.comraw.githubusercontent.com[!TIP]
api.github.comis blocked because GitHub API access uses the built-in GitHub tools by default. Instead of addingapi.github.comtonetwork.allowed, usetools.github.mode: gh-proxyfor direct pre-authenticated GitHub CLI access without requiring network access toapi.github.com:See GitHub Tools for more information on
gh-proxymode.To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.