diff --git a/README.md b/README.md index b6bb09e..64da83b 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ concurrency: jobs: triage: runs-on: ubuntu-latest - timeout-minutes: 120 + timeout-minutes: 240 environment: tamis permissions: contents: read diff --git a/docs/configuration.md b/docs/configuration.md index 279f783..e4ce79d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -31,9 +31,9 @@ The private artifact contains a full `.md` report with every duplicate proposed ## Resource budgets -An execution permits at most 800 provider requests and 256 MiB of cumulative request bodies. Each advisory shares 200 requests and 64 MiB across its reader run and comparisons; each agent token additionally permits 150 requests, not counting the refused routes Codex calls on every run. A request body cannot exceed 1 MiB. API responses request at most 16,000 output tokens; the ChatGPT backend has its own output limits. The thirty-minute advisory deadline also cancels host-side git commands. At most ten unpublished exact candidates are separately confirmed. At most ten reports and one report per known reporter are attempted per execution, and a report is only started while the execution still has a whole advisory budget, otherwise it waits for the next execution. SDK retries are disabled. +An execution permits at most 800 provider requests and 256 MiB of cumulative request bodies. Each advisory shares 200 requests and 64 MiB across its reader run and comparisons; each agent token additionally permits 150 requests, not counting the refused routes Codex calls on every run. A request body cannot exceed 1 MiB. API responses request at most 16,000 output tokens; the ChatGPT backend has its own output limits. The thirty-minute advisory deadline also cancels host-side git commands. At most ten unpublished exact candidates are separately confirmed. At most one report per known reporter is attempted per execution, and a report is only started while the execution still has a whole advisory budget and thirty minutes left of its 3 hours 50 minutes, otherwise it waits for the next execution. SDK retries are disabled, and an agent run that fails without a valid report is retried once after a minute. -These bounds limit each execution, not spending across scheduled runs. Use a dedicated provider account/project with an enforced spending or credit ceiling, restrict models, disable hosted tools and network allowlists there, and configure billing alerts. Provider pricing, tokenisation and ChatGPT credit policies differ, so byte/request limits are not a monetary guarantee. Keep a workflow `timeout-minutes` and a single concurrency group. Budget exhaustion leaves a report retriable rather than marking it permanently complete. +These bounds limit each execution, not spending across scheduled runs. Use a dedicated provider account/project with an enforced spending or credit ceiling, restrict models, disable hosted tools and network allowlists there, and configure billing alerts. Provider pricing, tokenisation and ChatGPT credit policies differ, so byte/request limits are not a monetary guarantee. Keep a workflow `timeout-minutes` of at least 240 and a single concurrency group. Budget exhaustion leaves a report retriable rather than marking it permanently complete. ## Project guidance diff --git a/internal/report/report.go b/internal/report/report.go index 413d1c2..dae6de1 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -63,6 +63,8 @@ type Report struct { // Duplicates come from the comparator, never from the reader. Duplicates []Duplicate `json:"-"` + // DuplicateSearchFailed means Duplicates is empty because the comparator did not finish. + DuplicateSearchFailed bool `json:"-"` // Policies are the files the policy check read, empty when the repository has none. Policies []string `json:"-"` // CreatedAt is when the advisory was reported. @@ -486,9 +488,12 @@ func RenderPrivate(r Report) string { b.WriteString(text(r.Summary) + "\n") b.WriteString("\n#### Possible duplicates\n\n") - if len(r.Duplicates) == 0 { + switch { + case r.DuplicateSearchFailed: + b.WriteString("The duplicate search could not complete.\n") + case len(r.Duplicates) == 0: b.WriteString("None found among the repository advisories.\n") - } else { + default: b.WriteString("| Advisory | Status | Confidence |\n|---|---|---|\n") } // Starting from the report's own date leaves every duplicate unmarked when the report is the oldest. @@ -546,7 +551,7 @@ func RenderPrivate(r Report) string { b.WriteString("Nothing in the policy seems to exclude this report.\n") } for _, p := range r.PolicyExclusions { - fmt.Fprintf(&b, "- %s: %s\n", text(p.Point), text(p.Reason)) + fmt.Fprintf(&b, "- %s: %s\n", strings.TrimSuffix(text(p.Point), "."), text(p.Reason)) } } diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 7b20130..85b09ba 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -323,6 +323,24 @@ func TestRenderPolicyWithoutExclusion(t *testing.T) { assert.Contains(t, got, "Checked against `.github/SECURITY.md`, `AGENTS.md`.\n\nNothing in the policy seems to exclude this report.") } +func TestRenderPolicyPointEndingWithAPeriod(t *testing.T) { + got := Render(Report{Policies: []string{"SECURITY.md"}, PolicyExclusions: []PolicyPoint{ + {Point: "Malicious inputs are not security issues.", Reason: "The PoC feeds `unserialize()`."}, + {Point: "Version 1.2 is unsupported", Reason: "It targets `1.2`."}, + }}) + + assert.Contains(t, got, "- Malicious inputs are not security issues: The PoC feeds `unserialize()`.\n") + assert.Contains(t, got, "- Version 1.2 is unsupported: It targets `1.2`.\n") + assert.NotContains(t, got, ".:") +} + +func TestRenderFailedDuplicateSearch(t *testing.T) { + got := Render(Report{Summary: "s", DuplicateSearchFailed: true}) + + assert.Contains(t, got, "#### Possible duplicates\n\nThe duplicate search could not complete.\n") + assert.NotContains(t, got, "None found among the repository advisories.") +} + func TestRenderEmpty(t *testing.T) { got := Render(Report{Summary: "Nothing"}) diff --git a/internal/triage/revisions.go b/internal/triage/revisions.go index 7b6ba06..ae52369 100644 --- a/internal/triage/revisions.go +++ b/internal/triage/revisions.go @@ -111,7 +111,7 @@ func triageRevisions(ctx context.Context, cfg Config, gh GitHub, reader Reader, duplicates, err := findDuplicates(ctx, path, reader, r, input, others, log) if err != nil { comparisonErr = err - privateNote = "\nThe duplicate search could not complete.\n" + r.DuplicateSearchFailed = true } else { matches := report.Report{Duplicates: duplicates} if err := (report.Checker{Advisories: byID}).Verify(ctx, &matches); err != nil { diff --git a/internal/triage/triage.go b/internal/triage/triage.go index 364f3f0..6fdf7cc 100644 --- a/internal/triage/triage.go +++ b/internal/triage/triage.go @@ -34,8 +34,9 @@ import ( // ProfilePath holds project-specific guidance, relative to the repository root. const ProfilePath = ".github/tamis.md" +var maxRunDuration = 3*time.Hour + 50*time.Minute + const ( - maxTriagesPerRun = 10 maxAdvisoryDuration = 30 * time.Minute maxComparatorRequests = 200 maxAttempts = 3 @@ -76,6 +77,31 @@ type Reader interface { Read(ctx context.Context, req sandbox.Request) (json.RawMessage, error) } +var retryDelay = time.Minute + +// retrying runs a failed agent once more after retryDelay, so a model provider briefly overloaded or dropping +// its stream does not cost a triage attempt. +type retrying struct { + Reader + log *slog.Logger +} + +func (r retrying) Read(ctx context.Context, req sandbox.Request) (json.RawMessage, error) { + raw, err := r.Reader.Read(ctx, req) + // An incomplete run already wrote a valid report, which the triage keeps. + if !errors.As(err, new(*sandbox.Error)) || errors.As(err, new(*sandbox.Incomplete)) { + return raw, err + } + r.log.Info("the agent run failed, retrying it", "delay", retryDelay) + select { + case <-ctx.Done(): + return raw, err + case <-time.After(retryDelay): + } + + return r.Reader.Read(ctx, req) +} + // Run logs which advisory it works on and its steps, never report content nor raw errors. The action only // runs from a private repository, so its logs stay with the security team. func Run(ctx context.Context, cfg Config, gh GitHub, reader Reader, eng engine.Engine, log *slog.Logger) error { @@ -106,7 +132,8 @@ func Run(ctx context.Context, cfg Config, gh GitHub, reader Reader, eng engine.E defer func() { _ = exec.Command("git", "-C", cfg.RepoPath, "worktree", "prune").Run() }() defer os.RemoveAll(sourceDir) - var triaged, skipped, failed, attempted int + var triaged, skipped, failed int + deadline := time.Now().Add(maxRunDuration) reporters := map[string]bool{} for _, a := range all { if !selected(cfg, a) { @@ -130,8 +157,9 @@ func Run(ctx context.Context, cfg Config, gh GitHub, reader Reader, eng engine.E skipped++ continue } - if attempted == maxTriagesPerRun { + if time.Until(deadline) < maxAdvisoryDuration { cancel() + log.Info("the run time is spent, the remaining advisories wait for the next run") break } if a.Author.Login != "" && reporters[strings.ToLower(a.Author.Login)] { @@ -146,11 +174,10 @@ func Run(ctx context.Context, cfg Config, gh GitHub, reader Reader, eng engine.E break } reporters[strings.ToLower(a.Author.Login)] = true - attempted++ log.Info("triaging", "attempt", failures+1) advisoryCtx, spent := usage.Track(engine.WithRequestLimit(advisoryCtx, maxComparatorRequests)) - if published, err := triageRevisions(advisoryCtx, cfg, gh, reader, eng, a, all, comments, login, revision, failures+1 >= maxAttempts, sourceDir, log); err != nil { + if published, err := triageRevisions(advisoryCtx, cfg, gh, retrying{reader, log}, eng, a, all, comments, login, revision, failures+1 >= maxAttempts, sourceDir, log); err != nil { cancel() log.Error("triage failed", "reason", describe(err), "usage", spent()) if keepAgentError(cfg, a.GHSAID, revision, err) != nil { diff --git a/internal/triage/triage_test.go b/internal/triage/triage_test.go index a8c4b0c..a81cdc0 100644 --- a/internal/triage/triage_test.go +++ b/internal/triage/triage_test.go @@ -114,6 +114,8 @@ type fakeEngine struct { fail string // crash makes fail look like an agent container exiting on an error. crash bool + // failures is how many calls matching fail fail before the next ones succeed, all of them when zero. + failures, failed int // incomplete makes the reader hand back its report with a failed agent run. incomplete bool reader string @@ -146,7 +148,8 @@ func (e *fakeEngine) Read(ctx context.Context, req sandbox.Request) (json.RawMes } } e.calls = append(e.calls, call) - if e.fail != "" && strings.Contains(call.prompt, e.fail) { + if e.fail != "" && strings.Contains(call.prompt, e.fail) && (e.failures == 0 || e.failed < e.failures) { + e.failed++ err := errors.New("model unavailable while reading SECRET-PAYLOAD from " + call.prompt) if e.crash { return nil, &sandbox.Error{Err: err} @@ -727,18 +730,32 @@ func TestRunRejectsADuplicateOutsideTheAdvisories(t *testing.T) { body := gh.posted["GHSA-new"] require.Contains(t, body, report.Marker, "a failed duplicate search blocked the comment") require.NotContains(t, body, "GHSA-invented") + require.Contains(t, body, "The duplicate search could not complete.") + require.NotContains(t, body, "None found among the repository advisories.") require.Contains(t, privateReports(t, gh), "The duplicate search could not complete.") } -func TestRunCapsNewAdvisoriesPerExecution(t *testing.T) { +func TestRunTriagesEveryNewAdvisoryWithinItsTime(t *testing.T) { gh, eng := newFixture(), &fakeEngine{reader: `{"summary":"s"}`} gh.advisories = nil - for i := 0; i < maxTriagesPerRun+1; i++ { + for i := range 11 { gh.advisories = append(gh.advisories, github.Advisory{GHSAID: fmt.Sprintf("GHSA-%d", i), State: "triage", CreatedAt: since.Add(time.Hour)}) } require.NoError(t, run(t, Config{Since: since}, gh, eng)) - require.Len(t, gh.posted, maxTriagesPerRun) - require.Len(t, eng.role(""), maxTriagesPerRun) + require.Len(t, gh.posted, 11) + require.Len(t, eng.role(""), 11) +} + +func TestRunLeavesTheAdvisoriesToTheNextRunOnceItsTimeIsSpent(t *testing.T) { + defer func(d time.Duration) { maxRunDuration = d }(maxRunDuration) + maxRunDuration = maxAdvisoryDuration + gh, eng := newFixture(), &fakeEngine{} + var logs strings.Builder + + require.NoError(t, runLogged(t, Config{Since: since}, gh, eng, &logs)) + require.Empty(t, gh.posted) + require.Empty(t, eng.role("")) + require.Contains(t, logs.String(), "the run time is spent, the remaining advisories wait for the next run") } func TestRunHidesPartialMatchesOnUnpublishedAdvisories(t *testing.T) { @@ -793,7 +810,56 @@ func TestRunReportsFailuresAndContinues(t *testing.T) { require.NotEmpty(t, gh.posted["GHSA-other"], "the other advisory should still be triaged") } +func noRetryDelay(t *testing.T) { + t.Helper() + delay := retryDelay + retryDelay = 0 + t.Cleanup(func() { retryDelay = delay }) +} + +func TestRunRetriesAFailedAgentRunOnce(t *testing.T) { + noRetryDelay(t) + gh, eng := newFixture(), &fakeEngine{fail: "GHSA-new", crash: true, failures: 1} + var logs bytes.Buffer + + require.NoError(t, runLogged(t, Config{Advisory: "GHSA-new"}, gh, eng, &logs)) + + require.Len(t, eng.role(""), 2) + require.Contains(t, gh.posted["GHSA-new"], report.Marker) + require.Contains(t, gh.posted["GHSA-new"], "Looks like a heap overflow.") + require.Contains(t, logs.String(), "the agent run failed, retrying it") +} + +func TestRunFailsWhenTheRetriedAgentRunFailsAgain(t *testing.T) { + noRetryDelay(t) + gh, eng := newFixture(), &fakeEngine{fail: "GHSA-new", crash: true} + + require.Error(t, run(t, Config{Advisory: "GHSA-new"}, gh, eng)) + + require.Len(t, eng.role(""), 2, "a failed agent run is retried once only") + require.Contains(t, gh.posted["GHSA-new"], report.FailedMarker) +} + +func TestRunDoesNotRetryAFailureOutsideTheAgentRun(t *testing.T) { + gh, eng := newFixture(), &fakeEngine{fail: "GHSA-new"} + var logs bytes.Buffer + + require.Error(t, runLogged(t, Config{Advisory: "GHSA-new"}, gh, eng, &logs)) + + require.Len(t, eng.role(""), 1) + require.NotContains(t, logs.String(), "retrying") +} + +func TestRunDoesNotRetryAnIncompleteAgentRun(t *testing.T) { + gh, eng := newFixture(), &fakeEngine{incomplete: true} + + require.Error(t, run(t, Config{Advisory: "GHSA-new"}, gh, eng)) + + require.Len(t, eng.role(""), 1) +} + func TestRunKeepsTheAgentErrorInThePrivateReports(t *testing.T) { + noRetryDelay(t) for name, crash := range map[string]bool{"agent crash": true, "other failure": false} { t.Run(name, func(t *testing.T) { gh, eng := newFixture(), &fakeEngine{fail: "GHSA-new", crash: crash}