Complete Go CLI command scraping - #4462
Conversation
|
Warning Review limit reachedNext included review available in 49 seconds. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (30)
📒 Files selected for processing (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe Go CLI scraper now traverses more commands, loads shared and test-specific help, parses options from usage and prose, preserves option shapes, and adds regression tests. The CLI documentation lists the newly covered commands. ChangesGo CLI scraper coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change expands Go CLI command and option generation coverage, including nested commands and shared flags. The supplied validation and regression coverage identify no current merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant GoCliScraper
participant GoCLI
participant HelpParser
participant OptionsGenerator
GoCliScraper->>GoCLI: Request command and shared help
GoCLI-->>GoCliScraper: Return help text and flag probe results
GoCliScraper->>HelpParser: Parse usage, option lines, and prose
HelpParser-->>OptionsGenerator: Return merged option definitions
OptionsGenerator-->>GoCliScraper: Produce command option records
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Failure
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c5f829caa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping
I ran a multi-angle review of GoCliScraper.cs (the only hand-edited source file) and cross-checked the highest-risk hypotheses against the real installed go help output (go1.24) and the actual generated *.Generated.cs diffs, so the findings below are evidence-backed rather than speculative. One initial hypothesis (that GetRepeatableOptions over-marks most go build flags as repeatable) was checked against the real go help build text and generated GoBuildOptions.Generated.cs, and is refuted — the "may be repeated" sentence sits in its own paragraph naming only -asmflags/-gccgoflags/-gcflags/-ldflags, which are correctly typed.
Blocking
1. Claiming a flag name into seenOptions before AddUsageOptions runs locks in wrong types for several real flags (GoCliScraper.cs ~L388-L520)
AddDocumentedOptions/AddProseOptions add a flag to seenOptions as soon as they classify it (right or wrong), which prevents AddUsageOptions from ever getting a chance to correct a wrong bool? classification using the command's own usage synopsis. Verified against real output:
go help mod download:usage: go mod download [-x] [-json] [-reuse=old.json] [modules]+ prose with no literal=.-reuseregresses from the pre-PR-correct[CliOption("-reuse", Format=EqualsSeparated)] string?to[CliFlag("-reuse")] bool?— callers can no longer pass a reuse-file path.go help list's-f(usage[-f format]) andgo help mod tidy's-compat(usage[-compat=version]) regress the same way.
This is the architectural root cause of most of the type-fidelity issues below: the scraper treats "first source that finds a flag" as authoritative instead of "most specific source." A more robust design would run all three sources (tabular flags, usage synopsis, prose) per-command first, collect candidate classifications with a confidence/specificity rank (usage synopsis with an explicit value token > tabular value hint > prose value mention > prose bare mention), and resolve conflicts by rank rather than by claim order. That removes the order-dependence entirely and would have caught -reuse, -f, and -compat correctly regardless of which paragraph or section Go happens to document them in.
2. KnownValueOptions is a hand-maintained allowlist that is already incomplete, and will keep drifting (GoCliScraper.cs ~L60-L66)
-buildvcs genuinely takes a value (-buildvcs=false/-buildvcs=auto, confirmed in real go help build prose) but has no bare-word hint on its own declaration line and isn't in the list, so it — and everything that splices in shared build flags (clean/get/install/list/run/test) — generates as [CliFlag] bool? instead of a string option, making -buildvcs=auto impossible to pass through the generated API.
This is a symptom of the same root cause as #1: a hardcoded allowlist is being used to patch over a classification heuristic that doesn't look at the option's own description text for an explicit -flag=value pattern. Since Go's own convention for documenting a value-taking flag with example usage is consistently -flag=value or -flag value somewhere in its own paragraph (even when it's not on the declaration line), scanning the entire accumulated description for that pattern before falling back to "no hint found → assume boolean" would eliminate the need for a hand-maintained list altogether, and would self-correct as Go's help text evolves instead of silently going stale.
3. ProseOptionParagraphPattern().Match() is a single, anchored match — later flags mentioned in the same paragraph lose their real description or are silently dropped (GoCliScraper.cs ~L500-L520)
Real go help fmt: "The -n flag prints commands that would be executed.\nThe -x flag prints commands as they are executed." is one paragraph (single \n, no blank line). Because the match is singular and anchored to the paragraph start, -n's description wrongly absorbs both sentences while -x falls through to the generic placeholder "The -x option." — confirmed in the generated diff. go version's -v flag is dropped entirely the same way because its sentence isn't at the paragraph's start.
Suggest switching to Matches() (all matches within the paragraph) instead of a single anchored Match(), iterating each sentence independently. This is a small, mechanical fix but meaningfully improves the fidelity of a large fraction of the generated XML docs.
Non-blocking / worth addressing
4. SharedBuildFlagCommands (GoCliScraper.cs ~L69-L74) duplicates a fact the code already derives dynamically one function later. UsesSharedBuildFlags/SharedBuildCommandsPattern correctly parses "The build flags are shared by the X, Y, Z commands:" straight from go help build's own text. ShouldLoadSharedBuildFlags's hardcoded 6-command list gates whether that dynamic check even runs, so if Go's shared-command set ever changes, the dynamic parser would track it correctly but the hardcoded gate wouldn't — a newly-added shared command would silently never get its build flags spliced in. Since the dynamic parse already exists, prefer relying on it as the single source of truth (e.g., always fetch/append go build's help for two-segment command paths and let UsesSharedBuildFlags decide inclusion) rather than maintaining two lists that can drift apart.
5. Redundant work: ParagraphSeparatorPattern().Split(helpText) runs twice per command (GetRepeatableOptions ~L397 and AddProseOptions ~L506), and BuildFlagsUsagePattern().IsMatch(helpText) runs twice per shared-build-flags command (ShouldLoadSharedBuildFlags ~L84 and UsesSharedBuildFlags ~L142). Neither is a correctness issue, but both are easy to fix by splitting/matching once in AddOptions/GetHelpTextAsync and threading the result through, rather than recomputing.
6. Duplicated type-selection ternary. AddDocumentedOptions and AddProseOptions each independently write acceptsMultipleValues ? "IEnumerable<string>?" : isFlag ? "bool?" : "string?" instead of calling the existing CliScraperBase.AsCSharpType(scalarType, acceptsMultipleValues). Two copies of the same rule means a future extension (e.g. a numeric type) has to be made in three places instead of one — worth consolidating onto the base helper while touching this file.
7. RepeatedOptionPattern ((?:may|can) be repeated|repeatable) is narrower than the base class's CliScraperBase.RepeatableValuePattern/DescriptionDeclaresRepeatableOption, which also matches phrasing like "supports multiple X" or "array of X". Since ValidateOptionShapes runs the base pattern against the option's generated Description text regardless of which scraper produced it, a Go option whose help text uses one of the broader phrasings that Go's local pattern doesn't recognize would leave AcceptsMultipleValues=false while the base validator's independent check considers it repeatable, throwing InvalidOperationException and aborting the whole Go regeneration run with no obvious message pointing at the mismatched regex. Not triggered by the currently-sampled go help text, but worth widening Go's pattern to a superset of (or delegating to) the base pattern so a future Go wording change doesn't break CI regeneration outright.
8. Go.CommandCoverage.json's committed toolVersion is "go version go1.27.0 windows/amd64", but GoCliScraper.GenerationPlatform is unchanged and still declares Linux. If CI or a contributor regenerates this file on the declared platform, the provenance string (and possibly formatting) will differ from what's committed, producing a spurious diff on the next legitimate regeneration. Worth regenerating on Linux before merge, or updating GenerationPlatform if Windows is now the intended generation platform.
None of the above are style nits — they're either real user-facing regressions in generated option types/docs (1-3) or drift risks that will bite silently later (4, 7, 8). Recommend addressing at least 1-3 before merge since they change public generated API surface (flag types flip from string-accepting to boolean-only), which is a breaking change for any caller currently passing -reuse=, -f, -compat=, or -buildvcs= values.
|
CI attempt 1 failed in unrelated core GC/lifetime test ExternalMetadata_DoesNotRootCollectibleConsumerAssembly (WeakReference remained alive); Go scraper changes do not touch generated runtime metadata. Re-running the failed job once as flaky infrastructure/test timing. |
GitHub Actions
Failure
3c5f829 to
4569b55
Compare
|
Addressed the full blocking review in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4569b55f3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (follow-up)
Re-reviewed at HEAD 4569b55f, which is the author's response to the prior blocking review (posted at 3c5f829c).
Previous findings — verified as fixed
I checked each of the previously-blocking items against the real generated output:
- Order-dependent
seenOptionslocking in wrong types — fixed.seenOptionsis gone;AddOrMergeOptionnow merges candidates from all three sources (usage synopsis, tabular docs, prose) bySwitchName, ANDingIsFlagso any source that finds a value hint wins. Confirmed-reuse(GoModDownloadOptions.Generated.cs:38),-f(GoListOptions.Generated.cs:26), and-compat(GoModTidyOptions.Generated.cs:56) are all correctlystring?/EqualsSeparatedagain. - Hand-maintained
KnownValueOptionsallowlist — removed, replaced byGetValueSeparatorscanning description text for an explicit-flag=value/-flag valuepattern. Confirmed-buildvcs(GoBuildOptions.Generated.cs:122) is nowstring?/EqualsSeparatedfrom the-buildvcs=falseexample in its own prose. - Single anchored
ProseOptionParagraphPattern().Match()dropping/merging sentences — fixed viaProseOptionSentencePattern().Matches()iterating each sentence independently. Confirmedgo fmt's-n/-x(GoFmtOptions.Generated.cs:26,32) andgo version's-v(GoVersionOptions.Generated.cs:31) all have distinct, correct descriptions now. - Hardcoded
SharedBuildFlagCommandslist drifting from the dynamic parser — fixed.UsesSharedBuildFlags(GoCliScraper.cs:125-149) now derives shared-command membership purely from parsinggo build's own "shared by X, Y, Z commands" sentence; a new test (Loads_Only_Dynamically_Shared_Build_Flags) locks this in.
5-8 (redundant computation, duplicated ternary, narrow repeatability regex, staletoolVersionplatform) all addressed too — paragraphs are split once and threaded through,AsCSharpTypeis used consistently, repeatability delegates to the base class'sDescriptionDeclaresRepeatableOption, andGo.CommandCoverage.jsonnow sayslinux/amd64.
Good, thorough response to the prior review — this is exactly the kind of source-ranking fix that was suggested.
New blocking issue introduced by the fix
GetValueSeparator's "does a value token follow the flag" regex has a blocklist that's too narrow, and it scans the whole merged paragraph instead of the declaring sentence, causing two flags to regress from boolean to string-valued (GoCliScraper.cs:575-590, called from AddProseOptions at GoCliScraper.cs:520)
return Regex.IsMatch(
description,
$@"(?<![\w-])-{optionPattern}\s+(?!(?:flag|flags|option|options)\b)\S+",
RegexOptions.IgnoreCase)This only excludes the flag being followed by the literal words flag/flags/option/options. Any other connector word right after the flag name — is, was, used, specified, set, given — is treated as evidence the flag takes a value. Because AddProseOptions passes the entire paragraph (not just the sentence that declared the flag) into this check, a flag correctly identified as boolean by its declaring sentence can still get downgraded by an unrelated later sentence in the same paragraph.
Two real, verifiable regressions from this in the current generated output:
go version -json: real help text is "The -json flag is similar to -m but outputs the runtime/debug.BuildInfo in JSON format. If flag -json is specified without -m, go version reports an error." — a boolean toggle, same shape as-m(correctlyCliFlag).-json -is specifiedmatches the regex (isisn't excluded), so it's generated as[CliOption("-json")] string?(GoVersionOptions.Generated.cs:38) instead of[CliFlag("-json")] bool?.go get -tool: "The -tool flag instructs go to add a matching tool line to go.mod for each listed package. If -tool is used with @none, the line will be removed." — same pattern (-tool is used), generated as[CliOption("-tool")] string?(GoGetOptions.Generated.cs:38) instead of a boolean flag.
Both are functionally wrong regardless of input: Go's flag parser doesn't expect a value after -json/-tool, so any string this generated option is set to gets emitted as an extra token Go will misinterpret (e.g. as a stray file/package argument), rather than toggling the flag.
This is the same class of problem as the original finding #2 (hand-maintained heuristics drifting from reality) — except now it's a blocklist instead of an allowlist, which is just as fragile in the opposite direction: it says "assume value-taking unless the following word is one of these four", when the actual signal for "this flag takes a value" should be a positive one. A more robust design:
- Scope
GetValueSeparator's scan to the declaring sentence (sentence, already available inAddProseOptions) rather thannormalizedParagraph— this alone would have caught neither-jsonnor-toolregressing, since their declaring sentences ("The -json flag is similar to -m...", "The -tool flag instructs go to add...") don't contain a value-token pattern. - Even scoped to the sentence, a blocklist of connector words will keep drifting as Go's prose wording varies. Prefer a positive signal instead: only treat text after
-flagas a value hint when it looks like an actual meta-variable/example (e.g. quoted, wrapped in backticks, or matching the same value-hint shapeAddUsageOptions/AddDocumentedOptionsalready use elsewhere), rather than "any word that isn't in this short exclude-list."
Given the PR's stated goal is fixing exactly this class of flag-type fidelity issue, and the regression flips two more flags from boolean to string-valued (a breaking change to the generated public API, same as the issues fixed in this commit), I'd treat this as blocking before merge.
GitHub Actions
Failure
4569b55 to
50a13b1
Compare
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50a13b1409
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at 50a13b14)
Re-reviewed at HEAD 50a13b1409, the author's response to the second blocking review (posted at 4569b55f).
Previous finding — verified fixed
The GetValueSeparator blocklist regression (-json/-tool wrongly downgraded from boolean to string) is fixed. GetValueSeparator no longer uses a "followed by any non-excluded word" heuristic; the new pattern requires the value token itself to be followed by flag/option (GoCliScraper.cs:631-637), which no longer misfires on connector words like "is"/"used". Confirmed in generated output: GoVersionOptions.Generated.cs:38 (-json) and GoGetOptions.Generated.cs:38 (-tool) are both back to [CliFlag] bool? with correct, distinct descriptions.
New blocking issues at this HEAD
1. AddOrMergeOption matches SwitchName case-insensitively, merging distinct flags that differ only by case (GoCliScraper.cs:644-645)
var existingIndex = options.FindIndex(option =>
option.SwitchName.Equals(candidate.SwitchName, StringComparison.OrdinalIgnoreCase));Go itself treats -c and -C as different flags. go test has its own boolean -c ("compile the test binary … but do not run it"), while the shared build-flags text appended from go help build has -C dir ("Change to dir before running the command…"). Because the merge is case-insensitive, these collide. Confirmed in GoTestOptions.Generated.cs:26-30: [CliOption("-c")] public string? C { get; set; } carries the -C build flag's description verbatim ("Change to dir before running the command. Any files named on the command line are interpreted after changing directories…"). go test's real -c is gone — it's unreachable as a boolean, and its actual meaning is lost — while -C dir is also inaccessible under its own name. Switching the comparison to StringComparison.Ordinal would preserve both flags as distinct options, which better reflects Go's actual flag semantics (case-sensitive) than deduping by case-insensitive name.
2. AddOrMergeOption always lets the newer non-empty candidate description win, regardless of source quality (GoCliScraper.cs:659-661)
Description = string.IsNullOrWhiteSpace(candidate.Description)
? existing.Description
: candidate.Description,This is "last write wins" rather than "best source wins" — a later, loosely-matched prose sentence silently replaces a concise, correct per-flag description that a more specific source (tabular docs / usage synopsis) had already set. Confirmed in GoBuildOptions.Generated.cs:106-159: Asmflags, Gccgoflags, Gcflags, and Ldflags all share one identical ~965-character run-on paragraph as their XML summary (from the "-asmflags, -gccgoflags, -gcflags, and -ldflags flags accept a space-separated list…" prose overwriting their earlier concise descriptions), and the same pattern repeats in GoModEditOptions.Generated.cs:59-141 for -require, -droprequire, -exclude, -dropexclude, -replace, -dropreplace, -retract, -dropretract, -tool, -droptool, -ignore, -dropignore — all get the identical boilerplate "may be repeated" sentence instead of their own distinct descriptions. This is the same class of bug as the two already-fixed order-dependence issues (candidate classification and value-separator detection) — worth applying the same fix pattern here: prefer the more specific/structured source's description (tabular/usage) over a later prose match, rather than plain overwrite-by-arrival-order.
3. AccumulateMultiLineDescription's continuation check doesn't recognize the indentation go help testflag actually uses, dropping nearly all test-flag descriptions (GoCliScraper.cs:703-707)
var leadingSpaces = nextLine.Length - nextLine.TrimStart().Length;
if (!nextLine.StartsWith("\t\t", StringComparison.Ordinal) && leadingSpaces < 8)
{
break;
}go help testflag (the help source this PR newly scrapes via GetTestFlagsHelp, GoCliScraper.cs:76-99) indents continuation lines with one tab + 4 spaces (e.g. \t-count n\n\t Run each test, benchmark, and fuzz seed n times…), which is 5 leading characters — neither \t\t nor >= 8. The loop breaks immediately, so the description stays empty. Confirmed in GoTestOptions.Generated.cs: -bench, -count, -run, -timeout, -shuffle, -skip, -parallel, -cpu, -fuzz, -fuzztime, -list, -outputdir, -vet, -blockprofile*, -cpuprofile, -memprofile*, -mutexprofile*, -trace, -o, -exec all render with no <summary> at all — which defeats the purpose of the testflag-scraping feature this PR adds. Since Go's help output mixes indentation conventions across subcommands (some use \t\t, testflag uses \t + spaces), the continuation check should key off "more indented than the flag declaration line" rather than a specific fixed prefix/width.
None of these are style nits — all three are user-facing regressions in the generated public API surface: #1 makes a real flag (go test -c) unreachable and mislabels another, #2 silently corrupts documentation for a wide swath of generated options across multiple commands, and #3 defeats this PR's own stated goal (completing Go CLI scraping) for nearly every go test flag. Recommend addressing before merge.
|
Addressed the current blocking review in b2818c4:
Local validation: OptionsGenerator Release build, Go solution Release build, 14/14 focused scraper tests, and changed-file whitespace verification all pass. Canonical Linux Go regeneration is running at https://github.com/thomhurst/ModularPipelines/actions/runs/33496045679; it will create a child PR if Linux provenance/output differs. |
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3a4096100
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9eab39700
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at f9eab397)
Re-reviewed at HEAD f9eab3970059bdecd560e08806ba703dc8e07c1b, after three prior rounds of blocking review (3c5f829c → 4569b55f → 50a13b14 → f9eab397). I re-verified every previously-blocking finding against the actual generated output and, where relevant, against real go help text from a locally installed go1.24.
Previous findings — all verified fixed
- Order-dependent classification / hand-maintained allowlists / single anchored prose match (round 1):
AddOrMergeOptionnow merges candidates from usage synopsis, tabular docs, and prose bySwitchName, andKnownValueOptionsis gone in favor ofGetValueSeparatorscanning description text. Confirmed correct for-reuse,-f,-compat,-buildvcs. GetValueSeparatorblocklist regression (round 2): replaced with a narrower positive-match pattern; confirmed-json(GoVersionOptions.Generated.cs) and-tool(GoGetOptions.Generated.cs) are correctlybool?again.- Case-insensitive
SwitchNamemerge colliding-c/-C(round 3):AddOrMergeOptionnow compares withStringComparison.Ordinal(GoCliScraper.cs:707). ConfirmedGoTestOptions.Generated.cshas a correct boolean-cand a separate-C/COptionwith the shared build-flag description andEarlyOperandphase. - "Last write wins" description overwrite corrupting shared-flag docs (round 3):
GetPreferredDescriptionnow only lets a later prose match win when it uniquely names one flag (optionMatches.Length == 1) or the existing description is empty/generic. Confirmed-asmflags/-gccgoflags/-gcflags/-ldflags(GoBuildOptions.Generated.cs) and themod editflag pairs each have distinct, correct descriptions again — no more shared boilerplate paragraph. AccumulateMultiLineDescriptionmissinggo help testflag's indentation convention (round 3): now compares continuation-line indentation relative to the flag's own declaration line instead of a hardcoded width/prefix (GoCliScraper.cs:781-785). Confirmed-bench,-count,-run,-timeout,-shuffle, etc. inGoTestOptions.Generated.csall have full, correct descriptions now.f9eab397itself (this HEAD's own fix) restores-C/-overlayvalue-shape and-C'sEarlyOperandphase forgo tool, and widensProseOptionParagraphPattern/ProseOptionSentencePatternfrom hardcoded"Edit also provides the"to"[A-Z][\w-]* also provides the"so"Tool also provides the -C, -overlay, and -modcacherw build flags."is recognized. Verified inGoToolOptions.Generated.cs— all three flags present with correct shapes.
Each fix addressed the root cause the earlier reviews pointed at (rank-by-specificity merging, ordinal comparison, relative indentation) rather than patching the specific failing input, which is the right call — it's what let a single fix cover multiple previously-unseen commands (e.g. go tool) instead of needing another band-aid.
Non-blocking observation (new, minor, doc-fidelity only — not a type/API regression)
ProseOptionParagraphPattern/ProseOptionSentencePattern only recognize a flag declaration sentence that begins with "The" or "<Word> also provides the". Go also documents flags with "If the -x flag is given/set, ...", which isn't recognized. Confirmed against real go help output:
go help mod why: "If the -m flag is given, why treats the arguments as a list of modules..." →GoModWhyOptions.Generated.cs's-mstill falls back to the generic placeholder"The -m option."even though-m's real description is one sentence away.go help run: "If the -exec flag is given, 'go run' invokes the binary using xprog..." →GoRunOptions.Generated.cs's-exechas the same placeholder instead of its real description.
Both flags still get the correct type (-m is bool?/CliFlag, -exec is string?/CliOption) since usage-synopsis parsing independently determines shape — this is a documentation-quality gap, not a functional regression, and only 2 flags were affected across every command this PR scrapes (verified by grepping all scraped commands' real help text for the If/When/Unless the -X flag phrasing). Worth extending the sentence-lead-in pattern to a small set of alternatives (The|If the|When the|Unless the|<Word> also provides the) the same way f9eab397 just extended it for <Word> also provides the, so future regenerations pick up the real description instead of the placeholder — but not worth blocking this PR over.
Other checks
Go.CommandCoverage.jsoncorrectly showscommandCount: 32,toolVersion: "go version go1.27.0 linux/amd64"(platform fixed per round-1 finding #8).- No empty or dangling XML doc summaries found across any
*.Generated.csfile in this PR. - Test file (
GoCliScraperTests.cs) has dedicated cases for case-sensitivity (Preserves_Case_Sensitive_Go_Flags), dynamic shared-build-flag membership (Loads_Only_Dynamically_Shared_Build_Flags), and shared-flag expansion (Expands_Shared_Build_Flags_For_Affected_Commands), which lock in the round-1/3 fixes against regression.
Good, thorough resolution across four review rounds — this is ready to merge as far as this review is concerned.
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a535c6145
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at 2a535c61)
Re-reviewed at HEAD 2a535c6145027d9abe27fe0afe4ab09f5f8cab75. The prior review (at f9eab397) was CLEAR; this round covers only the diff since then — commit 2a535c6145 ("fix(go): model optional and terminal values"), which touches GoCliScraper.cs, the scraper's own test suite, ModularPipelines.csproj, and adds a new test/ModularPipelines.Go.UnitTests/GoOptionsTests.cs.
Blocking: new test file doesn't compile — generated Options classes were not regenerated
GoCliScraper.ApplyCommandSpecificOptionShapes now reshapes two flags:
go get -u→ValueArity = Optional,CSharpType = "string?"(should driveCliOptionValue?perCliOptionDefinition.cs:54)go test -args→CSharpType = "IEnumerable<string>?",GroupValues = true, no longer a bare flag
But src/ModularPipelines.Go/Options/GoGetOptions.Generated.cs and GoTestOptions.Generated.cs were not regenerated in this commit — git diff between f9eab397 and 2a535c61 touches zero files under src/ModularPipelines.Go/. They still have the old shapes:
// GoGetOptions.Generated.cs
[CliOption("-u", Format = OptionFormat.EqualsSeparated)]
public string? U { get; set; }
// GoTestOptions.Generated.cs
[CliFlag("-args", Phase = CommandLinePhase.Terminal)]
public bool? Args { get; set; }The new GoOptionsTests.cs added in this same commit assumes the new shapes:
U = CliOptionValue.Bare, // CliOptionValue -> string? has no implicit conversion
...
Args = ["payload", "-test.v"], // collection expression -> bool? is not constructibleI built test/ModularPipelines.Go.UnitTests/ModularPipelines.Go.UnitTests.csproj (Release) against this HEAD to confirm, and it fails with:
GoOptionsTests.cs(17,17): error CS0029: Cannot implicitly convert type 'ModularPipelines.Models.CliOptionValue' to 'string'
GoOptionsTests.cs(44,20): error CS9174: Cannot initialize type 'bool?' with a collection expression because the type is not constructible.
This breaks the build for anyone/CI running the ModularPipelines.Go test project — not a style nit, a hard compile failure.
Fix: regenerate GoGetOptions.Generated.cs and GoTestOptions.Generated.cs (and check Go.CommandCoverage.json/any other generated Go artifacts) from the updated scraper so U becomes CliOptionValue? and Args becomes IEnumerable<string>? with the new attribute shape, matching what GoCliScraperTests.cs's new assertions (PropertyType == "CliOptionValue?", etc.) already expect from the scraper side. The scraper-level tests pass because they only assert on the intermediate CliOptionDefinition, not on the emitted C# — that gap is why this slipped through even though GoCliScraperTests.cs was updated correctly.
Suggestion (process, not this PR's code)
This is the fourth time across this PR's review rounds that a scraper-shape change and its consuming generated code have drifted out of sync (rounds 1–3 were about the parsing logic itself; this one is about the codegen step never being re-run after a shape change). Since regeneration is a manual step this repo can't run in CI-equivalent agent sandboxes, it may be worth a lightweight guard — e.g. a test that builds a CliOptionDefinition for go get -u/go test -args via the real scraper and asserts the emitted PropertyType matches what's actually checked into *.Generated.cs (a targeted golden-file diff), rather than relying on every contributor remembering to run the generator before adding hand-written tests against its output. That would have caught this at scraper-test time instead of at dotnet build time.
abdc892 to
d9d2479
Compare
|
Addressed both exact-head findings at
Validation: OptionsGenerator 1210/1210; Go 4/4; both scoped Release builds clean (0 warnings/errors); narrow whitespace checks clean. Generated files came from pinned Go workflow run 33504656183 / PR #4482. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d247977f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at 820f721a)
Re-reviewed at HEAD 820f721ac1adaff3739561ff2b8c0e9aef0d3c5f, covering the diff since the last review (2a535c61, since rebased to 0061e2510e in current history): d9d247977f ("chore: Update go CLI options (#4482)") and 820f721ac1 ("fix(go): preserve list option semantics").
d9d247977f correctly fixes the previously-flagged blocking issue: GoGetOptions.Generated.cs and GoTestOptions.Generated.cs are now regenerated to match the scraper's -u/-args shapes, and test/ModularPipelines.Go.UnitTests/GoOptionsTests.cs's existing assertions now compile against them.
Blocking: same regeneration gap reintroduced for go list -json
820f721ac1 changes GoCliScraper.cs (tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GoCliScraper.cs) to reshape go list's -json flag via the new ApplyOptionalValueOptionShape(options, "-json", "=") helper — turning it from a bare boolean flag into an optional-value option (CSharpType = "string?", ValueArity = Optional, ValueSeparator = "=").
But src/ModularPipelines.Go/Options/GoListOptions.Generated.cs was not regenerated in this commit (only GoOptionsTests.cs, GoCliScraperTests.cs, and the scraper itself changed). It still declares:
[CliFlag("-json")]
public bool? Json { get; set; }The same commit adds two new tests to GoOptionsTests.cs that assume the new shape:
Json = CliOptionValue.Bare, // CliOptionValue -> bool? : no implicit conversion
...
Json = "ImportPath,Name", // string -> bool? : no implicit conversionI built test/ModularPipelines.Go.UnitTests/ModularPipelines.Go.UnitTests.csproj (Release) at this HEAD to confirm, and it fails with:
GoOptionsTests.cs(57,20): error CS0029: Cannot implicitly convert type 'ModularPipelines.Models.CliOptionValue' to 'bool?'
GoOptionsTests.cs(70,20): error CS0029: Cannot implicitly convert type 'string' to 'bool?'
This is a hard compile failure for the ModularPipelines.Go test project, not a style nit.
Fix: regenerate GoListOptions.Generated.cs (and re-check Go.CommandCoverage.json/other generated Go artifacts) from the updated scraper so Json becomes CliOptionValue? with the -json/= optional-value shape, matching what the new GoCliScraperTests.cs test (List_Json_Accepts_Bare_And_Field_Values) already expects at the scraper level.
Process suggestion (repeating from the last round, now with a second occurrence)
This is now the fifth review round, and the second time specifically, where a scraper-shape change lands without its consuming *.Generated.cs being regenerated in the same commit — round 4 was -u/-args, this round is -json on go list. The pattern is consistent enough that it's worth treating as a process gap rather than a one-off oversight: GoCliScraperTests.cs validates the intermediate CliOptionDefinition, but nothing validates that the checked-in *.Generated.cs output actually reflects the scraper's current output. A lightweight golden-file/round-trip test — generate the Options class for a command from the real scraper and diff it against what's checked into src/ModularPipelines.Go/Options/*.Generated.cs — would catch this at scraper-test time instead of at dotnet build time, and would remove the dependency on every contributor remembering to run the generator by hand after touching shape logic.
|
Addressed the current-head review blockers in
Validation:
Go is unavailable locally, so current-output regeneration is running through the repository workflow: https://github.com/thomhurst/ModularPipelines/actions/runs/33732765132. It is configured to open a child PR against this branch if generated files change. |
|
Follow-up Verified against exact Go 1.27.1 help and local regeneration:
Corrected-head Linux regeneration is queued to update child PR #4542: https://github.com/thomhurst/ModularPipelines/actions/runs/33733870700 |
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at 4c00d22b)
Re-reviewed at HEAD 4c00d22b15262b61e07a1e0a15d7d1805c02971f, covering the diff since the last BLOCKING verdict (e1882bf9): commits 7cf9ff6e ("preserve case-sensitive flags") and 4c00d22b ("compare visual flag indentation"). Only GoCliScraper.cs and GoCliScraperTests.cs changed in this range.
Previous blocking findings — verified fixed
1. Case-sensitive dedup / case-insensitive PropertyName collision — Fixed by the new DisambiguatePropertyNames (GoCliScraper.cs:585-628). It groups the final option list by PropertyName, and for any group with more than one member, re-derives a case-preserving name per switch via EncodeSwitchName (-c → LowerC, -C → UpperC), deterministically ordered and collision-checked against a global usedPropertyNames set. The new Preserves_Case_Sensitive_Go_Flags test now asserts both PropertyName values and overall uniqueness (test.Options.Select(o => o.PropertyName).Distinct().Count() == test.Options.Count), which is real coverage for the silent-drop scenario. Confirmed correct.
2. AccumulateMultiLineDescription truncating prose starting with - — The blind trimmedNext.StartsWith('-') break is gone. New regression tests (Preserves_Hyphen_Led_Description_Continuations, Treats_Equivalent_Tab_Indentation_As_An_Option_Boundary) cover the previously-truncated cases. Confirmed correct.
New finding
Dead/redundant condition in the AccumulateMultiLineDescription fix (GoCliScraper.cs:1010-1021)
var continuationIndentation = GetIndentationWidth(nextLine);
if (GoOptionLinePattern().IsMatch(nextLine)
&& continuationIndentation <= declarationIndentation)
{
break;
}
if (continuationIndentation <= declarationIndentation)
{
break;
}The first if is unreachable as independent logic: it requires continuationIndentation <= declarationIndentation as part of its own condition, and the very next if breaks on that same condition alone. So whenever the GoOptionLinePattern branch would fire, the plain indentation branch right after it would already have fired for the identical reason — the pattern match never changes the outcome. In other words, this function's actual break behavior is 100% determined by indentation; GoOptionLinePattern().IsMatch(nextLine) is evaluated but its result is discarded.
Why this is worth fixing rather than leaving as a nitpick: this exact function has now caused two rounds of subtle, hard-to-spot correctness bugs (the -C collision and the StartsWith('-') truncation), each fixed only after generated output was manually diffed against real go help text. A dead condition here reads as if "genuine new flag declarations always end the description," which is not what the code does — only shallower-or-equal indentation ends it. A future contributor who trusts the apparent intent (e.g., removes the "redundant-looking" indentation check because "the option-line-pattern check already handles it") would silently reintroduce the exact truncation bug this PR just fixed, since the pattern check alone (without the indentation gate) was never wired up as a standalone condition.
Suggested fix — pick one and drop the other:
- Simplest: delete the dead block (lines 1010-1013 in isolation) and keep only the unconditional indentation check, since the tests already prove indentation alone is sufficient and this was
main's original (pre-regression) design. - Or, if the intent genuinely was "break on indentation drop, OR when the next line is unambiguously a new flag declaration regardless of indentation" (closer to what the prior review suggested), remove the
&& continuationIndentation <= declarationIndentationclause from the firstifso it's a real, independent condition instead of a strict subset of the second.
Either is a small change; right now the code has two checks doing the work of one, which obscures which mechanism actually governs correctness in a function that has already proven easy to get subtly wrong.
Not re-flagged (previously marked non-blocking, unaddressed, still non-blocking)
Items #3 (flags-section boundary now scans the whole help text), #4 (sequential shared-build-flag probing), and #5 (per-command shape overrides duplicating generic heuristics) from the e1882bf9 review remain as before — process/maintainability suggestions, not correctness issues, and out of scope for this re-review.
Co-authored-by: thomhurst <9139608+thomhurst@users.noreply.github.com>
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at 52517218)
Re-reviewed at HEAD 52517218911eb9747ceccea855b02000116596e5. The only change since the last BLOCKING verdict (4c00d22b) is 5251721891 ("chore: Update go CLI options (#4542)"), which touches only generated files (*.Generated.cs, Go.CommandCoverage.json) — no changes to GoCliScraper.cs or its tests.
Confirms prior fixes are working correctly
The regenerated output in 5251721891 is a good sanity check of the case-sensitivity and multi-line-description fixes from earlier rounds:
-c/-Congo docandgo testnow render as distinct properties (LowerC/UpperC) instead of colliding.- Previously truncated multi-line descriptions (e.g.
-buildvcs,-modfile,-fuzzminimizetime) now include their full trailing sentences instead of cutting off mid-sentence at a line starting with-.
These are consistent with the fixes verified in the 4c00d22b review and look correct.
Previous blocking finding — still unaddressed
Dead/redundant condition in AccumulateMultiLineDescription (tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GoCliScraper.cs:1010-1021, unchanged since the 4c00d22b review):
var continuationIndentation = GetIndentationWidth(nextLine);
if (GoOptionLinePattern().IsMatch(nextLine)
&& continuationIndentation <= declarationIndentation)
{
break;
}
if (continuationIndentation <= declarationIndentation)
{
break;
}The first if is unreachable as independent logic: its own condition already requires continuationIndentation <= declarationIndentation, and the very next if breaks on exactly that condition alone. So the GoOptionLinePattern().IsMatch(nextLine) check is evaluated but its result never changes control flow — the function's actual break behavior is 100% governed by indentation alone.
This isn't just a style nitpick: this exact function has already caused two rounds of subtle, hard-to-spot correctness regressions (the -C/-c collision, and the StartsWith('-') truncation this pattern-match code was meant to help fix), each only caught by manually diffing generated output against real go help text. Leaving the dead condition in place is actively misleading — it reads as "a genuine new flag declaration always ends the description," which is not what the code does. A future contributor who trusts that apparent intent (e.g. removes the "redundant" indentation check because "the pattern check already handles it") would silently reintroduce the exact truncation bug this PR fixed, since the pattern check alone, without the indentation gate, was never wired up as a standalone condition.
Suggested fix — pick one:
- Delete the dead block (lines 1010-1015 in isolation) and keep only the unconditional indentation check — the existing tests already prove indentation alone is sufficient, and this matches
main's original pre-regression design. - Or, if the intent is genuinely "break on indentation drop, OR when the next line is unambiguously a new flag declaration regardless of indentation," drop the
&& continuationIndentation <= declarationIndentationclause from the firstifso it becomes a real, independent condition instead of a strict subset of the second.
Either is a small, localized change. Right now two checks are doing the work of one in a function that has already proven easy to get subtly wrong — worth cleaning up before the next contributor "simplifies" it in the wrong direction.
Not re-flagged (previously marked non-blocking, still non-blocking)
Sequential shared-build-flag probing and per-command shape overrides duplicating generic heuristics remain process/maintainability suggestions, not correctness issues, and are unchanged since the last review.
GitHub Actions
Failure
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at 17088322)
Re-reviewed at HEAD 170883228175fb6fecb8dfb8cbdaba51a791da6f. The only change since the last BLOCKING verdict (52517218) is 1708832281 ("refactor(go): remove redundant option check"), which touches only GoCliScraper.cs.
Previous blocking finding — fixed
The dead/redundant condition in AccumulateMultiLineDescription (flagged in the 52517218 review) is resolved. The commit removes exactly the unreachable block:
var continuationIndentation = GetIndentationWidth(nextLine);
-if (GoOptionLinePattern().IsMatch(nextLine)
- && continuationIndentation <= declarationIndentation)
-{
- break;
-}
-
if (continuationIndentation <= declarationIndentation)
{
break;This is the first of the two suggested fixes from the prior review: the function's break behavior is now governed solely by the unconditional indentation check, with no misleading pattern-match condition left in place to confuse a future contributor. GoOptionLinePattern() remains used elsewhere (command-listing traversal and option-line parsing), so nothing is left dangling. The change is a pure deletion, scoped to exactly the flagged lines, with no other files touched — consistent with "small, localized change" from the prior recommendation.
Not re-flagged (previously marked non-blocking, still non-blocking)
Sequential shared-build-flag probing and per-command shape overrides duplicating generic heuristics remain process/maintainability suggestions, not correctness issues, and are unchanged since the last review.
No outstanding issues remain from this review history.
GitHub Actions
Inactive
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at b0fafbbb)
Re-reviewed at HEAD b0fafbbb34bad4112f2001948edd5ceda02f86e0. Since the last CLEAR review (17088322), the branch picked up two conflict-free merges of origin/main (e180ebf770 merging #4445, and b0fafbbb34 merging #4544/#4541/#4553/#4555/#4526) — no new commits were authored on this PR itself.
Diffed every file this PR owns (GoCliScraper.cs, GoCliScraperTests.cs, all src/ModularPipelines.Go/** generated options/services, GoEditOperation.cs, docs/docs/mp-packages/cli/go.md, GoOptionsTests.cs, GeneratedOptionsSmokeTestHarness.cs + its tests, and ModularPipelines.csproj) between 17088322 and b0fafbbb: the diff is empty. The merged-in main commits only touch unrelated files (Kubernetes/Cosign/Newman/Yq generated options, OptionsClassGenerator.cs, distributed-dispatch code, etc.) with no overlap against this PR's files, so no conflict resolution was needed and nothing here needs re-review.
No outstanding issues remain from this review history.
There was a problem hiding this comment.
Review: #4462 — Complete Go CLI command scraping (re-review at d9a35586)
Re-reviewed at HEAD d9a35586fa1b17cc443697f0b850df6ab49017c1. Since the last CLEAR verdict (b0fafbbb), the branch picked up one more conflict-free merge of origin/main (per the author's comment, syncing to register required CI contexts) — no new commits were authored on this PR itself.
Diffed every file this PR owns (GoCliScraper.cs, GoCliScraperTests.cs, all src/ModularPipelines.Go/** generated options/services, GoEditOperation.cs, docs/docs/mp-packages/cli/go.md, GoOptionsTests.cs, GeneratedOptionsSmokeTestHarness.cs + its tests, and ModularPipelines.csproj) between b0fafbbb and d9a35586:
git diff b0fafbbb34bad4112f2001948edd5ceda02f86e0 d9a35586fa1b17cc443697f0b850df6ab49017c1 -- src/ModularPipelines.Go tools/ModularPipelines.OptionsGenerator test/ModularPipelines.Go.UnitTests docs/docs/mp-packages/cli/go.md src/ModularPipelines/ModularPipelines.csproj test/ModularPipelines.TestHelpers/GeneratedOptionsSmokeTestHarness.cs test/ModularPipelines.UnitTests/Helpers/GeneratedOptionsSmokeTestHarnessTests.cs
Output is empty. The merged-in main commits only touch unrelated generated packages (Flux, Grype, Minikube, Nbgv, Pnpm, Shellcheck, Trivy, WinGet) plus CLAUDE.md/a minikube doc — no overlap with any file this PR owns, so this is a pure no-op merge from this PR's perspective and nothing here needs re-review.
Conclusion
All previously-blocking findings across this PR's many review rounds (order-dependent classification, case-insensitive switch merging, regeneration gaps between scraper-shape changes and generated *.Generated.cs files, the AddOrMergeOption AND-merge downgrade for go doc -c/-u, the PropertyName case-collision silent drop, and the multi-line-description truncation/dead-condition issues) were already resolved and verified fixed in prior rounds, most recently reconfirmed CLEAR at 17088322 and b0fafbbb. This round confirms the intervening merge introduced no new Go-scraper changes and no regressions.
The two carried-over non-blocking suggestions remain open but don't block merge:
GetSupportedFlagsAsyncstill probes shared build flags sequentially rather than viaTask.WhenAll.IsDocWithoutDirectFlagsstill hand-listsgo docas a special case rather than folding it into the generic flag-probing path.
GitHub Actions
Failure
|
Synced current |
GitHub Actions
Inactive
|
The dispatched Go regeneration (run 33999217019) generated successfully but failed in the "Synchronize generated public API baselines" step: |
|
Review: #4462 — Complete Go CLI command scraping (re-review at Re-reviewed at HEAD What I checked
This is the same "no-op sync merge" pattern seen repeatedly earlier in this PR's history ( Conclusion No actionable findings. All previously-blocking issues (order-dependent classification, case-insensitive switch merging, regeneration gaps between scraper-shape changes and generated |
1 participant
Closes #4323.
go mod edit/go work editoperationsValidation:
Broad format verification remains blocked by pre-existing unrelated formatting/analyzer findings, beginning with
IHelpTextCache.cs.Summary by CodeRabbit
New Features
go doc.Documentation