Skip to content

Commit b441796

Browse files
authored
Merge branch 'main' into docs/audit-fixes
2 parents e5458ce + 3d3dcfc commit b441796

3 files changed

Lines changed: 265 additions & 8 deletions

File tree

‎.github/workflows/simplycubed.yml‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,18 @@ jobs:
112112
# which is the CLI's default base. The CLI resolves that case on its own
113113
# now, but naming the branch the event came from is exact rather than
114114
# inferred.
115-
run: simplycubed run ${{ inputs['dry-run'] && '--dry-run' || '' }} "${GITHUB_REPOSITORY}#${{ inputs['issue-number'] }}" --actor "${{ inputs['actor-login'] }}" --model "${{ inputs.model }}" --base "origin/${{ github.event.repository.default_branch }}" --repo-dir .
115+
env:
116+
COMMENT_BODY: ${{ inputs['comment-body'] }}
117+
# A comment goes through the parser, which recognises a fixed vocabulary
118+
# and does nothing otherwise. Without this every comment beginning with
119+
# the mention would start a full run, including "help" and including a
120+
# comment asking it NOT to proceed.
121+
run: |
122+
if [ -n "${COMMENT_BODY}" ]; then
123+
simplycubed command ${{ inputs['dry-run'] && '--dry-run' || '' }} --body "${COMMENT_BODY}" "${GITHUB_REPOSITORY}#${{ inputs['issue-number'] }}" --actor "${{ inputs['actor-login'] }}" --model "${{ inputs.model }}" --base "origin/${{ github.event.repository.default_branch }}" --repo-dir .
124+
else
125+
simplycubed run ${{ inputs['dry-run'] && '--dry-run' || '' }} "${GITHUB_REPOSITORY}#${{ inputs['issue-number'] }}" --actor "${{ inputs['actor-login'] }}" --model "${{ inputs.model }}" --base "origin/${{ github.event.repository.default_branch }}" --repo-dir .
126+
fi
116127
117128
address:
118129
if: ${{ inputs['pr-number'] != '' }}
@@ -166,4 +177,11 @@ jobs:
166177
GH_TOKEN: ${{ steps.app-token.outputs.token }}
167178
AZURE_OPENAI_ENDPOINT: ${{ inputs['azure-openai-endpoint'] }}
168179
AZURE_OPENAI_API_KEY: ${{ secrets['azure-openai-api-key'] }}
169-
run: simplycubed address ${{ inputs['dry-run'] && '--dry-run' || '' }} "${GITHUB_REPOSITORY}#${{ inputs['pr-number'] }}" --actor "${{ inputs['actor-login'] }}" --model "${{ inputs.model }}" --repo-dir .
180+
env:
181+
COMMENT_BODY: ${{ inputs['comment-body'] }}
182+
run: |
183+
if [ -n "${COMMENT_BODY}" ]; then
184+
simplycubed command ${{ inputs['dry-run'] && '--dry-run' || '' }} --body "${COMMENT_BODY}" "${GITHUB_REPOSITORY}#${{ inputs['pr-number'] }}" --actor "${{ inputs['actor-login'] }}" --model "${{ inputs.model }}" --repo-dir .
185+
else
186+
simplycubed address ${{ inputs['dry-run'] && '--dry-run' || '' }} "${GITHUB_REPOSITORY}#${{ inputs['pr-number'] }}" --actor "${{ inputs['actor-login'] }}" --model "${{ inputs.model }}" --repo-dir .
187+
fi

‎cmd/simplycubed/main.go‎

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -206,24 +206,47 @@ func newRunner(cfg *config.Config, codexHome string) engine.Runner {
206206
return r
207207
}
208208

209+
// takeBody removes --body (in either form) from argv and returns it with the
210+
// remaining arguments.
211+
func takeBody(argv []string) (body string, rest []string, err error) {
212+
for i := 0; i < len(argv); i++ {
213+
a := argv[i]
214+
switch {
215+
case a == "--body" || a == "-body":
216+
if i+1 >= len(argv) {
217+
return "", nil, fmt.Errorf("--body requires a value")
218+
}
219+
body = argv[i+1]
220+
i++
221+
case strings.HasPrefix(a, "--body="), strings.HasPrefix(a, "-body="):
222+
_, v, _ := strings.Cut(a, "=")
223+
body = v
224+
default:
225+
rest = append(rest, a)
226+
}
227+
}
228+
return body, rest, nil
229+
}
230+
209231
// commandCmd routes a comment addressed to the agent to the matching loop. The
210232
// comment body is untrusted, so it is parsed into a fixed vocabulary here and
211233
// never interpreted: an unrecognised comment does nothing at all.
212234
func commandCmd(argv []string, stdout io.Writer) error {
213-
fs := flag.NewFlagSet("command", flag.ContinueOnError)
214-
body := fs.String("body", "", "the comment body that triggered this run")
215-
rest, err := parseInterleaved(fs, argv)
235+
// --body is pulled out by hand and everything else is forwarded untouched,
236+
// because run and address own the rest of the flags. Parsing them here would
237+
// mean maintaining a second copy of that flag set.
238+
body, rest, err := takeBody(argv)
216239
if err != nil {
217240
return err
218241
}
219-
switch command.Parse(*body) {
242+
switch command.Parse(body) {
220243
case command.Help:
221244
fmt.Fprintln(stdout, command.HelpText)
222245
return nil
223246
case command.Go:
224-
return runCmd(append([]string{}, rest...))
247+
return runCmd(rest)
225248
case command.Address:
226-
return addressCmd(append([]string{}, rest...))
249+
return addressCmd(rest)
227250
default:
228251
fmt.Fprintln(stdout, "no command recognised in that comment; nothing to do")
229252
return nil

‎cmd/simplycubed/main_test.go‎

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ import (
44
"bytes"
55
"context"
66
"flag"
7+
"github.com/simplycubed/code/internal/config"
8+
"github.com/simplycubed/code/internal/domain"
9+
"github.com/simplycubed/code/internal/engine/claude"
10+
"github.com/simplycubed/code/internal/engine/codex"
711
"github.com/simplycubed/code/internal/forge/dryrun"
812
forgefake "github.com/simplycubed/code/internal/forge/fake"
913
vcsgit "github.com/simplycubed/code/internal/vcs/git"
@@ -492,6 +496,17 @@ func TestEngineEnvRejectsAnUnparseableEndpoint(t *testing.T) {
492496
}
493497
}
494498

499+
func TestDispatchRoutesCommand(t *testing.T) {
500+
var out, errOut bytes.Buffer
501+
// help is the one command that needs nothing else configured.
502+
if code := dispatch([]string{"command", "--body", "@simplycubed-code help"}, &out, &errOut); code != 0 {
503+
t.Fatalf("exit = %d, want 0", code)
504+
}
505+
if !strings.Contains(out.String(), "@simplycubed-code go") {
506+
t.Fatalf("stdout = %q, want the help text", out.String())
507+
}
508+
}
509+
495510
func TestDispatch(t *testing.T) {
496511
// Every command's exit code is part of the contract: a workflow step
497512
// distinguishes "you asked for something that does not exist" (2) from
@@ -756,3 +771,204 @@ func TestInitWithWorkflowIsIdempotent(t *testing.T) {
756771
t.Fatalf("the adopter's edit must survive: %q %v", got, err)
757772
}
758773
}
774+
775+
func TestTakeBody(t *testing.T) {
776+
// The other flags belong to run and address and must survive untouched,
777+
// or routing a comment would drop --actor and skip authorization.
778+
for name, tc := range map[string]struct {
779+
argv []string
780+
wantBody string
781+
wantRest []string
782+
}{
783+
"separate value": {
784+
[]string{"--body", "@simplycubed-code go", "--actor", "me", "o/r#1"},
785+
"@simplycubed-code go", []string{"--actor", "me", "o/r#1"},
786+
},
787+
"equals form": {
788+
[]string{"--body=@simplycubed-code address", "--dry-run", "o/r#2"},
789+
"@simplycubed-code address", []string{"--dry-run", "o/r#2"},
790+
},
791+
"absent": {
792+
[]string{"--actor", "me", "o/r#1"}, "", []string{"--actor", "me", "o/r#1"},
793+
},
794+
} {
795+
body, rest, err := takeBody(tc.argv)
796+
if err != nil {
797+
t.Fatalf("%s: %v", name, err)
798+
}
799+
if body != tc.wantBody {
800+
t.Fatalf("%s: body = %q, want %q", name, body, tc.wantBody)
801+
}
802+
if strings.Join(rest, " ") != strings.Join(tc.wantRest, " ") {
803+
t.Fatalf("%s: rest = %v, want %v", name, rest, tc.wantRest)
804+
}
805+
}
806+
if _, _, err := takeBody([]string{"--body"}); err == nil {
807+
t.Fatal("a --body with no value must error rather than silently parse as empty")
808+
}
809+
}
810+
811+
// The parser exists so that a comment the agent does not understand does
812+
// nothing. Before this was wired, any comment beginning with the mention
813+
// started a full run, including one asking it not to.
814+
func TestCommandCmdDoesNotRunOnUnrecognisedComments(t *testing.T) {
815+
for _, body := range []string{
816+
"@simplycubed-code please do not do this one",
817+
"thanks @simplycubed-code go",
818+
"@simplycubed-code",
819+
} {
820+
var out bytes.Buffer
821+
// A repo-dir that has no config would make a real run fail loudly, so
822+
// reaching one is detectable.
823+
err := commandCmd([]string{"--body", body, "--repo-dir", t.TempDir(), "o/r#1"}, &out)
824+
if err != nil {
825+
t.Fatalf("%q should be a quiet no-op, got: %v", body, err)
826+
}
827+
if !strings.Contains(out.String(), "nothing to do") {
828+
t.Fatalf("%q should report nothing to do, got: %q", body, out.String())
829+
}
830+
}
831+
}
832+
833+
// The routing branches are the point of the subcommand: a recognised verb has
834+
// to reach the matching loop, carrying the flags with it. Each case uses a
835+
// directory with no config, so reaching the loop fails at config load - which
836+
// is how we know it was reached at all.
837+
func TestCommandCmdRoutesRecognisedVerbsToTheirLoops(t *testing.T) {
838+
t.Setenv("AZURE_OPENAI_ENDPOINT", "https://r.openai.azure.com")
839+
t.Setenv("AZURE_OPENAI_API_KEY", "k")
840+
841+
for name, body := range map[string]string{
842+
"go routes to run": "@simplycubed-code go",
843+
"address routes to address": "@simplycubed-code address",
844+
} {
845+
var out bytes.Buffer
846+
err := commandCmd([]string{"--body", body, "--repo-dir", t.TempDir(), "o/r#1"}, &out)
847+
if err == nil {
848+
t.Fatalf("%s: expected the loop to be reached and fail on the missing config", name)
849+
}
850+
if !strings.Contains(err.Error(), "load config") {
851+
t.Fatalf("%s: err = %v, want a config error proving the loop was reached", name, err)
852+
}
853+
if strings.Contains(out.String(), "nothing to do") {
854+
t.Fatalf("%s: a recognised verb must not be treated as a no-op", name)
855+
}
856+
}
857+
}
858+
859+
func TestCommandCmdRejectsABodyFlagWithNoValue(t *testing.T) {
860+
if err := commandCmd([]string{"--body"}, io.Discard); err == nil {
861+
t.Fatal("expected an error for --body with no value")
862+
}
863+
}
864+
865+
// Engine selection is a documented config key and had no test. The default
866+
// matters most: an existing repository must keep the engine it already had.
867+
func TestNewRunnerSelectsTheEngine(t *testing.T) {
868+
if _, ok := newRunner(&config.Config{}, t.TempDir()).(*codex.Runner); !ok {
869+
t.Fatal("the default engine must be codex")
870+
}
871+
if _, ok := newRunner(&config.Config{Engine: "claude"}, t.TempDir()).(*claude.Runner); !ok {
872+
t.Fatal("engine: claude must select the Claude adapter")
873+
}
874+
// An unrecognised engine falls back rather than failing later with an
875+
// error that says nothing about the cause.
876+
if _, ok := newRunner(&config.Config{Engine: "nope"}, t.TempDir()).(*codex.Runner); !ok {
877+
t.Fatal("an unknown engine must fall back to the default")
878+
}
879+
}
880+
881+
// The sandbox knob exists so an adopter with externally sandboxed runners can
882+
// widen it themselves. Nothing in this repository sets it, so the test asserts
883+
// both that the default is untouched and that an explicit value is honoured.
884+
func TestNewRunnerHonoursTheSandboxOverride(t *testing.T) {
885+
r, ok := newRunner(&config.Config{}, t.TempDir()).(*codex.Runner)
886+
if !ok {
887+
t.Fatal("expected the codex runner")
888+
}
889+
if r.Sandbox != "workspace-write" {
890+
t.Fatalf("the sandbox must stay on by default, got %q", r.Sandbox)
891+
}
892+
t.Setenv("SIMPLYCUBED_SANDBOX", "read-only")
893+
r2, _ := newRunner(&config.Config{}, t.TempDir()).(*codex.Runner)
894+
if r2.Sandbox != "read-only" {
895+
t.Fatalf("an explicit sandbox mode must be honoured, got %q", r2.Sandbox)
896+
}
897+
}
898+
899+
// A local run has no bot identity to attribute commits to, and must not invent
900+
// one; an Actions run does, and a runner has no git identity without it.
901+
func TestNewVCSAttributesCommitsToTheCredential(t *testing.T) {
902+
if v := newVCS(""); v.AuthorName != "" || v.AuthorEmail != "" {
903+
t.Fatalf("a run with no known identity must not invent one: %+v", v)
904+
}
905+
v := newVCS("simplycubed-code[bot]")
906+
if v.AuthorName != "simplycubed-code[bot]" {
907+
t.Fatalf("AuthorName = %q", v.AuthorName)
908+
}
909+
if !strings.HasSuffix(v.AuthorEmail, "@users.noreply.github.com") || strings.Contains(v.AuthorEmail, "[bot]") {
910+
t.Fatalf("AuthorEmail = %q, want a noreply address with the bot suffix stripped", v.AuthorEmail)
911+
}
912+
}
913+
914+
func TestIsReleaseVersion(t *testing.T) {
915+
for in, want := range map[string]bool{
916+
"0.1.6": true, "10.20.30": true,
917+
"0.1": false, "0.1.6-rc1": false, "": false, "a.b.c": false, "0..6": false,
918+
} {
919+
if got := isReleaseVersion(in); got != want {
920+
t.Fatalf("isReleaseVersion(%q) = %v, want %v", in, got, want)
921+
}
922+
}
923+
}
924+
925+
// The issue title and body become the prompt, so fetching them wrong is not a
926+
// cosmetic failure: the agent would work from the wrong instructions.
927+
func TestFetchIssue(t *testing.T) {
928+
if runtime.GOOS == "windows" {
929+
t.Skip("gh stub is a POSIX shell script")
930+
}
931+
stubDir := t.TempDir()
932+
stub := filepath.Join(stubDir, "gh")
933+
body := `{"title":"Fix the parser","body":"It drops the last line."}`
934+
if err := os.WriteFile(stub, []byte("#!/bin/sh\ncat <<'J'\n"+body+"\nJ\n"), 0o755); err != nil {
935+
t.Fatal(err)
936+
}
937+
t.Setenv("PATH", stubDir+string(os.PathListSeparator)+os.Getenv("PATH"))
938+
939+
iss := domain.Issue{Repo: "o/r", Number: 12}
940+
if err := fetchIssue(&iss); err != nil {
941+
t.Fatalf("unexpected error: %v", err)
942+
}
943+
if iss.Title != "Fix the parser" || iss.Body != "It drops the last line." {
944+
t.Fatalf("issue = %+v", iss)
945+
}
946+
947+
// Malformed output must be an error, not a silently empty prompt.
948+
if err := os.WriteFile(stub, []byte("#!/bin/sh\necho 'not json'\n"), 0o755); err != nil {
949+
t.Fatal(err)
950+
}
951+
if err := fetchIssue(&domain.Issue{Repo: "o/r", Number: 1}); err == nil {
952+
t.Fatal("malformed gh output must error")
953+
}
954+
955+
// A gh failure must surface rather than leaving the issue blank.
956+
if err := os.WriteFile(stub, []byte("#!/bin/sh\nexit 1\n"), 0o755); err != nil {
957+
t.Fatal(err)
958+
}
959+
if err := fetchIssue(&domain.Issue{Repo: "o/r", Number: 1}); err == nil {
960+
t.Fatal("a gh failure must error")
961+
}
962+
}
963+
964+
func TestRunAndAddressRequireARef(t *testing.T) {
965+
t.Setenv("AZURE_OPENAI_ENDPOINT", "https://r.openai.azure.com")
966+
t.Setenv("AZURE_OPENAI_API_KEY", "k")
967+
repo := repoWithConfig(t)
968+
for name, fn := range map[string]func([]string) error{"run": runCmd, "address": addressCmd} {
969+
err := fn([]string{"--repo-dir", repo, "--state-dir", t.TempDir()})
970+
if err == nil || !strings.Contains(err.Error(), "missing") {
971+
t.Fatalf("%s with no ref: err = %v, want a missing-ref error", name, err)
972+
}
973+
}
974+
}

0 commit comments

Comments
 (0)