fix: apply 13 lint-monster correctness/style findings (2026-06-29)#42165
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis already completed in a prior invocation of this workflow run. Comment and APPROVE review were successfully posted to PR #42165 (score: 100/100, 1 design test, 0 violations). No further action needed. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42165 does not have the 'implementation' label and has only 15 new lines of code in business logic directories (threshold: 100). This is a lint/style fix PR with no significant architectural decisions. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small lint-driven correctness/style updates across Go utilities/parsing/constants, plus minor formatting refactors in the agentic workflows dashboard extension.
Changes:
- Replaced several
len(s) > 0checks withs != ""(including guarding string indexing sites). - Optimized schema error logging to count lines without allocating a
[]string. - Improved test robustness by failing fast on
json.Unmarshalerrors; also reformatted parts of the dashboard extension HTML/JS.
Show a summary per file
| File | Description |
|---|---|
| pkg/stringutil/stringutil.go | Uses normalized != "" before appending a trailing newline. |
| pkg/stringutil/sanitize.go | Uses result != "" before indexing result[0]. |
| pkg/parser/schema_errors.go | Uses strings.Count(...)+1 for line count logging to avoid Split allocation. |
| pkg/parser/remote_fetch.go | Uses trimmed != "" before indexing trimmed[0]. |
| pkg/parser/json_path_locator_test.go | Adds explicit json.Unmarshal error handling in the test. |
| pkg/parser/import_error.go | Uses line != "" before checking line[0] to detect section exit. |
| pkg/parser/frontmatter_content.go | Uses line != "" / word != "" checks in metadata parsing and name generation. |
| pkg/constants/version_constants.go | Uses v != "" for semantic-string validity checks. |
| pkg/constants/url_constants.go | Uses d != "" for semantic-string validity checks. |
| pkg/constants/job_constants.go | Uses j != "" / s != "" for semantic-string validity checks. |
| pkg/constants/constants.go | Uses c != "" for semantic-string validity checks. |
| .github/extensions/agentic-workflows-dashboard/web/index.html | Collapses a few conditional rows into single-line markup. |
| .github/extensions/agentic-workflows-dashboard/extension.mjs | Refactors formatting of execFile, Promise.all callsite, and refresh handler block. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Low
| try { | ||
| if (pathname === "/" || pathname === "/index.html") { | ||
| const [html, css] = await Promise.all([ | ||
| readFile(join(__dirname, "web", "index.html"), "utf8"), | ||
| readFile(join(__dirname, "web", "styles.css"), "utf8"), | ||
| ]); | ||
| const [html, css] = await Promise.all([readFile(join(__dirname, "web", "index.html"), "utf8"), readFile(join(__dirname, "web", "styles.css"), "utf8")]); | ||
| res.setHeader("Content-Type", "text/html; charset=utf-8"); |
| if err := json.Unmarshal([]byte(schemaJSON), &schemaDoc); err != nil { | ||
| t.Fatalf("json.Unmarshal error: %v", err) | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Change detail: The PR fixes a lint finding — Test inflation note: The production file Verdict
References:
|
There was a problem hiding this comment.
Review
All 13 lint-monster fixes look correct and safe.
Go changes — every len(x) > 0 → x != "" replacement is a semantically equivalent string comparison, safe at all call sites including those guarding x[0] indexing. The strings.Count(message, "\n")+1 optimization is arithmetically equivalent to len(strings.Split(message, "\n")) for all inputs and avoids a []string allocation in a log-only call. The json.Unmarshal error surfacing in the test is a reliability improvement.
No security, correctness, or reliability issues found.
Minor nit (non-blocking)
In extension.mjs, the execFile and refresh handler blocks were each expanded to multi-line for readability, but the Promise.all([readFile(...), readFile(...)]) call was compressed onto one long line — inconsistent with the rest of the formatting changes in this PR. No functional impact.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 34.2 AIC · ⌖ 6.94 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out — 2 observations, neither blocking. The lint-monster fixes are solid and the json.Unmarshal error handling is a genuine correctness win.
📋 Key Themes & Highlights
Key Themes
- Misleading optimization claim (
schema_errors.go): Thestrings.Count()+1change is idiomatic Go, but it doesn't actually avoid an allocation becausestrings.Splitis called unconditionally on the very next line. The PR description overstates the benefit. - Inconsistent formatting (
extension.mjs): ThePromise.allarray was collapsed to a single long line, contradicting the multi-line expansion applied toexecFileandhandlerin the same file.
Positive Highlights
- ✅ The
json.Unmarshalerror check injson_path_locator_test.gois a genuine correctness fix — silent test failures are dangerous. - ✅ All 11
len(s) > 0→s != ""replacements are correct idiomatic Go; the short-circuit semantics are preserved whereline[0]accesses follow the guard. - ✅ The typed-string
IsValid()methods (CommandPrefix,JobName,StepID,DocURL,Version) now consistently use!= ""— clean and readable. - ✅ The
execFileandhandlerreformats inextension.mjsgenuinely improve readability.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 47.2 AIC · ⌖ 7.47 AIC · ⊞ 6.6K
Comment /matt to run again
| } | ||
|
|
||
| schemaErrorsLog.Printf("Simplifying oneOf error message (%d lines)", len(strings.Split(message, "\n"))) | ||
| schemaErrorsLog.Printf("Simplifying oneOf error message (%d lines)", strings.Count(message, "\n")+1) |
There was a problem hiding this comment.
[/zoom-out] The allocation optimization is nullified by the very next statement.
The PR description says this change avoids allocating a []string just to count lines, but strings.Split(message, "\n") is called unconditionally on line 95 to populate lines. The strings.Count call saves nothing in practice because the []string allocation still happens immediately after.
💡 When this optimization would matter
This approach is a genuine win when the count is used in a conditional path that doesn't always reach the Split call:
// Worth optimizing: count guards the Split
if strings.Count(message, "\n")+1 > threshold {
lines := strings.Split(message, "\n")
// ...
}Here, however, lines is always assigned right after the log line, so the split always runs. The change is still valid idiomatic Go — but the performance rationale in the PR description is misleading.
@copilot please address this.
| readFile(join(__dirname, "web", "index.html"), "utf8"), | ||
| readFile(join(__dirname, "web", "styles.css"), "utf8"), | ||
| ]); | ||
| const [html, css] = await Promise.all([readFile(join(__dirname, "web", "index.html"), "utf8"), readFile(join(__dirname, "web", "styles.css"), "utf8")]); |
There was a problem hiding this comment.
[/zoom-out] Readability regression: Promise.all was collapsed to a single long line, contradicting the expansion applied to execFile and handler in the same PR.
The original multi-line form was clearer and consistent with this PR's other formatting improvements. The new single-line form is harder to scan at a glance.
💡 Suggested formatting
const [html, css] = await Promise.all([
readFile(join(__dirname, "web", "index.html"), "utf8"),
readFile(join(__dirname, "web", "styles.css"), "utf8"),
]);This mirrors the before-state and keeps each readFile call on its own line, which is easier to read and diff.
@copilot please address this.
There was a problem hiding this comment.
All Go changes are correct and safe to merge.
Review details
Correctness verification
len(s) > 0→s != ""(×11): Semantically equivalent for all named string types in Go; both returnfalsefor the zero value.strings.Count(message, "\n")+1: Mathematically identical tolen(strings.Split(message, "\n"))for every possible input —Spliton a separator always yields exactlyCount+1elements.- Index-safe guards (
line != "" && line[0] != ...): Equivalent to the removedlen(line) > 0 && ...guards; no index-out-of-bounds risk introduced. - Test
json.Unmarshalerror handling: Strictly better — previously a bad JSON constant would silently produce a nilschemaDoc, causing a misleading downstream failure. - Dashboard changes: Formatting-only; no behavioral risk.
🔎 Code quality review by PR Code Quality Reviewer · 67.8 AIC · ⌖ 6.86 AIC · ⊞ 5.2K
Comment /review to run again
🤖 PR Triage — §28357644191
Small, clean correctness fixes (+37/-38, 13 files): error handling in test, 11× string comparison normalisations. CI ✅ passing, 3 automated approvals. Ready to auto-merge.
|
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
Addresses 13 small correctness/style diagnostics flagged by the custom lint scanner across
pkg/stringutil,pkg/constants, andpkg/parser.Changes
pkg/parser/json_path_locator_test.go— handle thejson.Unmarshalerror instead of silently discarding itlen(s) > 0→s != ""across:pkg/stringutil/stringutil.go,pkg/stringutil/sanitize.gopkg/constants/constants.go,job_constants.go(×2),url_constants.go,version_constants.gopkg/parser/frontmatter_content.go(×2),import_error.go,remote_fetch.gopkg/parser/schema_errors.go— replacelen(strings.Split(message, "\n"))withstrings.Count(message, "\n")+1to avoid allocating a[]stringjust to count linesAll changes are pure expression replacements with no behavior change.