Compare commits

..

4 Commits

Author SHA1 Message Date
Rodin 64d82cd561 nit(#154): add t.Fatal guard if baseSubprocessArgs flag not found
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 26s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 53s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m10s
Address sonnet NIT: if --repo or --pr is ever removed from
baseSubprocessArgs(), the mutation loop silently no-ops and the test
becomes meaningless. Adding a found guard and t.Fatal makes the
regression immediately visible.
2026-05-15 08:00:27 -07:00
Rodin 2892dff95d refactor(#154): use flag-name lookup in InvalidRepo and InvalidPRNumber tests
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 37s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m1s
Make the override code robust against base-arg order changes: search for
the flag name ('--repo', '--pr') and replace the following value, rather
than searching for the value string itself.

This prevents a subtle bug if the same value appeared elsewhere in the
base args before the target flag.
2026-05-15 01:51:19 -07:00
Rodin e718cb841a refactor(#154): extract baseSubprocessArgs helper in main_test.go subprocess tests
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 44s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m40s
Reduces duplication across 7 subprocess test functions that repeated the
same set of required flags verbatim. When required flags change, only
baseSubprocessArgs() needs updating.

Changes:
- Add baseSubprocessArgs() helper (analogous to cleanEnv())
- InvalidReviewerName, InvalidTemperature, InvalidProvider,
  ConflictingPersonaFlags: use append(baseSubprocessArgs(), ...)
- InvalidRepo: mutate a copy from baseSubprocessArgs() to set invalid --repo
- InvalidPRNumber: mutate a copy from baseSubprocessArgs() to set invalid --pr
- MissingLLMBaseURL, MissingAICoreCredentials, DeprecatedGiteaURLEnv:
  keep inline args (intentionally omit base flags); add comment explaining why
- All 5 old tests migrated from deprecated --gitea-url to canonical --vcs-url

All subprocess tests pass. go vet clean.
2026-05-15 01:50:15 -07:00
rodin 30fe48d265 docs(#148): add SKILL.md and dev-loop-spec.md for dispatch redesign (#149)
CI / test (push) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
2026-05-15 08:12:02 +00:00
+60 -56
View File
@@ -880,16 +880,9 @@ func TestMainSubprocess_MissingFlags(t *testing.T) {
func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
os.Args = append(baseSubprocessArgs(),
"--reviewer-name", "invalid name",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
}
)
main()
return
}
@@ -908,15 +901,20 @@ func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
func TestMainSubprocess_InvalidRepo(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--repo", "invalidrepo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
args := baseSubprocessArgs()
// Replace the canonical --repo value with an invalid one.
found := false
for i, a := range args {
if a == "--repo" && i+1 < len(args) {
args[i+1] = "invalidrepo"
found = true
break
}
}
if !found {
t.Fatal("baseSubprocessArgs() does not contain --repo; test is broken")
}
os.Args = args
main()
return
}
@@ -935,15 +933,20 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) {
func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "notanumber",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
args := baseSubprocessArgs()
// Replace the canonical --pr value with a non-numeric string.
found := false
for i, a := range args {
if a == "--pr" && i+1 < len(args) {
args[i+1] = "notanumber"
found = true
break
}
}
if !found {
t.Fatal("baseSubprocessArgs() does not contain --pr; test is broken")
}
os.Args = args
main()
return
}
@@ -962,16 +965,9 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
func TestMainSubprocess_InvalidTemperature(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
os.Args = append(baseSubprocessArgs(),
"--llm-temperature", "5.0",
}
)
main()
return
}
@@ -990,16 +986,9 @@ func TestMainSubprocess_InvalidTemperature(t *testing.T) {
func TestMainSubprocess_InvalidProvider(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--gitea-url", "http://localhost",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "http://localhost",
"--llm-api-key", "key",
"--llm-model", "model",
os.Args = append(baseSubprocessArgs(),
"--llm-provider", "invalid-provider",
}
)
main()
return
}
@@ -1015,6 +1004,25 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) {
}
}
// baseSubprocessArgs returns the base set of required flags for subprocess tests
// that need a fully-configured main() invocation. Each test appends its own
// test-specific flags on top of this base.
//
// Using a helper here means that when the set of required flags changes, only
// this function needs updating (instead of every test that passes all flags).
func baseSubprocessArgs() []string {
return []string{
"review-bot",
"--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "https://api.example.com",
"--llm-api-key", "key",
"--llm-model", "gpt-4",
}
}
// cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS env vars that would
// interfere with testing missing-flag scenarios.
func cleanEnv() []string {
@@ -1389,13 +1397,14 @@ func TestFetchPatterns_MultipleRepos(t *testing.T) {
func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
// Note: cannot use baseSubprocessArgs() here because --llm-base-url and
// --llm-api-key are intentionally omitted to test the missing-URL error.
os.Args = []string{"review-bot",
"--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-model", "gpt-4",
// --llm-base-url and --llm-api-key intentionally omitted
}
main()
return
@@ -1417,6 +1426,8 @@ func TestMainSubprocess_MissingLLMBaseURL(t *testing.T) {
func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
// Note: cannot use baseSubprocessArgs() here because aicore provider
// does not require --llm-base-url / --llm-api-key; those are omitted.
os.Args = []string{"review-bot",
"--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo",
@@ -1446,17 +1457,10 @@ func TestMainSubprocess_MissingAICoreCredentials(t *testing.T) {
func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
os.Args = []string{"review-bot",
"--vcs-url", "https://gitea.example.com",
"--repo", "owner/repo",
"--pr", "1",
"--reviewer-token", "tok",
"--llm-base-url", "https://api.example.com",
"--llm-api-key", "key",
"--llm-model", "gpt-4",
os.Args = append(baseSubprocessArgs(),
"--persona", "security",
"--persona-file", "custom.json",
}
)
main()
return
}
@@ -1477,9 +1481,9 @@ func TestMainSubprocess_ConflictingPersonaFlags(t *testing.T) {
func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
// Set required flags but omit --vcs-url; GITEA_URL should be picked up.
// The test will exit with an error after VCS init (no PR to fetch), but
// the deprecation warning must appear.
// Note: cannot use baseSubprocessArgs() here because --vcs-url must be
// omitted — this test verifies that GITEA_URL env var is picked up as a
// deprecated fallback when --vcs-url is absent.
os.Args = []string{"review-bot",
// No --vcs-url: should fall back to GITEA_URL env var
"--repo", "owner/repo",