rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106

Merged
aweiker merged 17 commits from review-bot-issue-82 into feature/github-support 2026-05-13 17:16:28 +00:00
Conversation 98 Commits 17 Files Changed 14
+3 -2
1 changed files with 3 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 91fba770d9 - Show all commits
cmd/review-bot/main.go
+3 -2
View File
8
 @@ -85,8 +85,9 @@ func main() {
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
rodin marked this conversation as resolved
sonnet-review-bot commented 2026-05-13 11:34:32 +00:00
Review
Copy Link
Copy Source

[NIT] The --gitea-url alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: flag.StringVar shares the pointer, so whichever flag is set last wins. The current comment is accurate but its length makes it easy to miss the ORDERING constraint.

**[NIT]** The `--gitea-url` alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: `flag.StringVar shares the pointer, so whichever flag is set last wins.` The current comment is accurate but its length makes it easy to miss the ORDERING constraint.
// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The --gitea-url alias registration reads *vcsURL at registration time (before flag.Parse()). At that point *vcsURL holds the default value from envOrDefault(...), so the alias default is correct. However, if a user passes --vcs-url foo on the command line, the alias's default is stale — only the variable is shared, not a live default. This is acceptable because the flag package updates the pointed-to variable via flag.StringVar, so any value written by either --vcs-url or --gitea-url goes to the same *string. But the help text for --gitea-url will show the env-var default rather than reflecting --vcs-url's current value, which is mildly confusing. Consider a brief comment explaining the aliasing semantics.

**[MINOR]** The `--gitea-url` alias registration reads `*vcsURL` at registration time (before `flag.Parse()`). At that point `*vcsURL` holds the default value from `envOrDefault(...)`, so the alias default is correct. However, if a user passes `--vcs-url foo` on the command line, the alias's default is stale — only the variable is shared, not a live default. This is acceptable because the flag package updates the pointed-to variable via `flag.StringVar`, so any value written by either `--vcs-url` or `--gitea-url` goes to the same `*string`. But the help text for `--gitea-url` will show the env-var default rather than reflecting `--vcs-url`'s current value, which is mildly confusing. Consider a brief comment explaining the aliasing semantics.
sonnet-review-bot commented 2026-05-13 15:02:31 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The comment on the --gitea-url alias registration says "The *vcsURL dereference captures the env-var-resolved default at registration time". This is accurate but the behaviour when both --vcs-url and --gitea-url are passed (last one wins) is subtle enough that a brief integration test would provide more confidence than a comment alone.

**[NIT]** The comment on the `--gitea-url` alias registration says "The *vcsURL dereference captures the env-var-resolved default at registration time". This is accurate but the behaviour when both `--vcs-url` and `--gitea-url` are passed (last one wins) is subtle enough that a brief integration test would provide more confidence than a comment alone.
sonnet-review-bot commented 2026-05-13 16:04:55 +00:00
Review
Copy Link
Copy Source

[MINOR] The --gitea-url backward-compatible alias is registered with flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...) where *vcsURL is the default value captured at the moment of the call — before flag.Parse(). This is correct as written because *vcsURL is still the default string at that point, but it creates a subtle ordering dependency: if vcsURL ever acquired a non-default value between its declaration and this flag.StringVar call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (Must stay after vcsURL declaration and before flag.Parse()), but the fragility is worth noting. A cleaner pattern would be flag.StringVar(vcsURL, "gitea-url", "", "...") or extracting the default value to a named constant.

**[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` where `*vcsURL` is the default value captured at the moment of the call — before `flag.Parse()`. This is correct as written because `*vcsURL` is still the default string at that point, but it creates a subtle ordering dependency: if `vcsURL` ever acquired a non-default value between its declaration and this `flag.StringVar` call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (`Must stay after vcsURL declaration and before flag.Parse()`), but the fragility is worth noting. A cleaner pattern would be `flag.StringVar(vcsURL, "gitea-url", "", "...")` or extracting the default value to a named constant.
// Shares vcsURL pointer; empty default avoids ordering dependency with vcsURL declaration.
flag.StringVar(vcsURL, "gitea-url", "", "Deprecated: use --vcs-url instead")
// Must use *vcsURL as default: StringVar sets *p=value at registration, so empty
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The --gitea-url alias registration uses *vcsURL as the default value at registration time. This works correctly because flag parsing hasn't happened yet, so *vcsURL holds the env-var-resolved default. However this is subtle — if the evaluation order ever changed (e.g., someone moves flag.Parse() above this line), the alias would silently use an empty string default instead. A brief comment noting this dependency on registration-before-parse order would help future readers.

**[NIT]** The `--gitea-url` alias registration uses `*vcsURL` as the default value at registration time. This works correctly because flag parsing hasn't happened yet, so `*vcsURL` holds the env-var-resolved default. However this is subtle — if the evaluation order ever changed (e.g., someone moves `flag.Parse()` above this line), the alias would silently use an empty string default instead. A brief comment noting this dependency on registration-before-parse order would help future readers.
// string would overwrite the env-resolved value from the --vcs-url declaration.
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
flag.Parse()
67