Repository navigation
test(#8037): save logs and a failure summary for every failed behaviour scenario - #8040
Conversation
|
🤖 Finished Review · ✅ Success · Started 2:08 PM UTC · Completed 2:25 PM UTC Commit: Runtime: pi · Model: openai/gpt-6.1-sol → gpt-6.1-sol · Effort: high · Cost: $2.87 |
Site previewPreview: https://df0681f0-site.fullsend-ai.workers.dev Commit: |
|
Risk Assessment: moderate (2/5) DetailsLarge diff size and elevated churn in behaviour test infrastructure are balanced by zero protected or security-sensitive paths, bot authorship, additive and revertible failure-logging logic, and complete alignment with issue #8037 criteria. Previous runRisk Assessment: moderate (2/5) DetailsLarge diff size and elevated churn in behaviour test infrastructure are balanced by zero protected or security-sensitive paths, bot authorship, additive and revertible failure-logging logic, and complete alignment with issue #8037 criteria. Previous run (2)Risk Assessment: moderate (2/5) DetailsLarge diff and active recent churn in behaviour test infrastructure are balanced by zero protected or security-sensitive paths, complete alignment with issue #8037 criteria, and additive, easily revertible failure-logging logic. Previous run (3)Risk Assessment: moderate (2/5) DetailsModerate risk reflecting test framework failure log collection across 16 files with notable churn and coupling in shared packages, balanced by zero protected paths, no CI workflow modifications, a bot author, and strong alignment with linked issue #8037. Previous run (4)Risk Assessment: moderate (2/5) DetailsModerate risk reflecting test framework failure log collection across 16 files with notable churn and coupling in shared packages, balanced by zero protected paths, no CI workflow modifications, a bot author, and strong alignment with linked issue #8037. Previous run (5)Risk Assessment: moderate (2/5) DetailsModerate risk reflecting test framework failure log collection across 16 files with notable churn and coupling in shared packages, balanced by zero protected paths, no CI workflow modifications, a bot author, and strong alignment with linked issue #8037. Previous run (6)Risk Assessment: moderate (2/5) DetailsModerate risk reflecting test framework failure log collection across 16 files with notable churn and coupling in shared packages, balanced by zero protected paths, no CI workflow modifications, a bot author, and strong alignment with linked issue #8037. Previous run (7)Risk Assessment: moderate (2/5) DetailsModerate risk reflecting test framework log collection changes across 12 files with recent churn and coupling, balanced by zero protected paths, no workflow modifications, and alignment with linked issue #8037. Previous run (8)Risk Assessment: moderate (2/5) DetailsModerate risk reflecting test framework log collection changes across 12 files with recent churn and coupling, balanced by zero protected paths, no workflow modifications, and alignment with linked issue #8037. Previous run (9)Risk Assessment: moderate (2/5) DetailsModerate risk score reflecting test framework log collection changes across 12 files with recent churn and coupling, balanced by zero protected paths, no workflow modifications, and complete alignment with linked issue #8037. Previous run (10)Risk Assessment: moderate (2/5) DetailsModerate risk score reflecting test framework log collection changes across 12 files with recent churn and coupling, balanced by zero protected paths, no workflow modifications, and complete alignment with the linked issue. |
|
Looks good to me Previous runReviewFindingsLow
Next steps:
Previous run (2)ReviewFindingsLow
Next steps:
Previous run (3)ReviewFindingsMedium
Low
Next steps:
Previous run (4)Looks good to me Previous run (5)ReviewFindingsMedium
Next steps:
Previous run (6)ReviewFindingsMedium
Next steps:
Previous run (7)ReviewFindingsMedium
Next steps:
Previous run (8)ReviewFindingsMedium
Next steps:
Previous run (9)Looks good to me Previous run (10)ReviewFindingsMedium
Low
Labels: The PR changes Go behaviour-test infrastructure and end-to-end failure diagnostics. Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
component/e2e
|
🤖 Finished Fix · ✅ Success · Started 2:27 PM UTC · Completed 2:34 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.63 |
- Deduplicate only complete log snapshots: a run that has not reached a terminal status, or whose log text embeds a per-job fetch failure, is no longer recorded as saved, so the After hook collects it again. - Redact fully formatted diagnostic messages before logging them. - Redact scenario and workflow names before slugging them into artifact directory names. - Describe an unavailable repository identity instead of claiming that no workflow runs exist. Not addressed: registering World.Token with the redactor is the pre-existing limitation tracked by #7948 and is left to that follow-up. Addresses #8040 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 1 (bot-triggered)Addressed the three medium findings and the api-contract low finding in debuglogs.go with tests. The low-severity #7948 redactor-registration finding is recorded as a disagreement because it is pre-existing and tracked separately. Go tests and vet pass for pkg/behaviourtest; golangci-lint and pre-commit were unavailable in the sandbox (pre-commit could not fetch hooks), so only gofmt, go vet and the secret scan ran. Fixed (4):
Disagreed (1):
Tests: passed CI inspections (6):
Conflict update: skipped; strategy Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 2:36 PM UTC · Completed 2:47 PM UTC Commit: Runtime: pi · Model: openai/gpt-6.1-sol → gpt-6.1-sol · Effort: high · Cost: $1.35 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
/fs-fix rebase and resolve conflicts |
|
🤖 Finished Fix · ✅ Success · Started 10:20 AM UTC · Completed 10:25 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.30 |
- Deduplicate only complete log snapshots: a run that has not reached a terminal status, or whose log text embeds a per-job fetch failure, is no longer recorded as saved, so the After hook collects it again. - Redact fully formatted diagnostic messages before logging them. - Redact scenario and workflow names before slugging them into artifact directory names. - Describe an unavailable repository identity instead of claiming that no workflow runs exist. Not addressed: registering World.Token with the redactor is the pre-existing limitation tracked by #7948 and is left to that follow-up. Addresses #8040 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 2 (human-triggered)Rebased the PR branch onto origin/main and resolved one docs conflict; no other changes made. Fixed (1):
Tests: passed CI inspections (1):
Next steps:
|
7b0164d to
d30c536
Compare
|
🤖 Review · ❌ Terminated · Started 10:27 AM UTC · Ended 10:40 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
83c37fd to
b42b842
Compare
|
🤖 Finished Review · ✅ Success · Started 7:07 AM UTC · Completed 7:21 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $3.29 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 7:23 AM UTC · Completed 7:29 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.60 |
- Keep an already-saved complete workflow log snapshot when a later fetch of the same run is partial, instead of overwriting it. - Register explicit infrastructure identifiers (E2E_GCP_*, CLOUDFLARE_ACCOUNT_ID) with the redactor alongside the name-suffix matches. - Sanitize console diagnostics: replace control characters and break up the workflow-command delimiter so forge error text cannot inject commands. - Wrap the log fetch error before preparing the debug dir so the empty-log diagnostic is not dropped when the directory fails. Note: pre-commit could not fetch hook repositories in the sandbox; gofmt, go vet and the behaviourtest tests were run directly. The unrelated internal/security SSRF test fails in the sandbox (DNS resolution). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 2 (bot-triggered)Addressed all four low-severity findings in debuglogs.go with regression tests. The behaviourtest tests, go vet and gofmt pass. pre-commit could not fetch hook repos in the sandbox, so gofmt and go vet were run directly. The commit body has one line 1 character over gitlint's 80-column limit; it was not amended because history rewrites are not authorized. Fixed (4):
Tests: passed CI inspections (6):
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 7:31 AM UTC · Completed 7:40 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $2.16 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 7:42 AM UTC · Completed 7:47 AM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.44 |
) registerSecretForms ignored the result of RegisterRuntimeSecret, which refuses values under 8 bytes, so a short sensitive value (World.Token or a sensitive environment variable) was left unredacted in summaries, console diagnostics, collected logs and directory names. Values the shared redactor declines (4-7 bytes) are now tracked locally and masked by redactText. Values under 4 bytes are still not masked, since doing so would erase common fragments of every artifact. Note: pre-commit hooks were not run via pre-commit (it could not fetch hook repositories: TLS failure). Secret scan, gofmt, go vet and go test ./pkg/behaviourtest/... pass; golangci-lint is not installed. go test ./internal/security fails on this sandbox with or without this change (DNS resolves a test hostname to a blocked address). Addresses #8040 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 3 (bot-triggered)Addressed the single low-severity finding by masking short (4-7 byte) sensitive literals locally in redactText, with a regression test. pre-commit could not fetch hook repos in the sandbox (TLS failure), so secret scan, gofmt, go vet and go test ./pkg/behaviourtest/... were run instead. The internal/security tests fail in this sandbox on the base commit too (DNS), unrelated to this change. Fixed (1):
Tests: passed CI inspections (5):
Conflict update: skipped; strategy Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 7:49 AM UTC · Completed 7:56 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.43 |
Superseded by updated review
fullsend-ai-review
Bot
added
the
ready-for-merge
rh-hemartin
added
the
ok-to-test
This branch was successfully deployed
1 participant
Summary
Makes every failed behaviour scenario leave an artifact in
BEHAVIOUR_ARTIFACT_DIR. That artifact is the logs of the workflow runs in the scenario's leased repository, or a summary saying why the logs could not be collected. Collection runs in the scenario After hook, before the repository is deleted.Related Issue
Fixes the uncovered failure paths left after 2a3da80 (#7876). Related: #6115 and #7948 (saved logs stay redacted).
Changes
saveWorkflowRunLogsonly ran when a step got a run object back. Nothing was saved when:WaitForHarnessAgent/WaitForWorkflowreturn no rundrainIssueOpenWorkflow,waitForDispatchRun)GetRunLogsfailed, since the error only went to the test logsteps.CollectFailureLogs(new,pkg/behaviourtest/steps/debuglogs.go), called fromafterScenariofor failed, non-skipped scenarios beforeCleanupScenarioandDeallocateRepo:debug-scenario-<name>-<suffix>/failure-summary.txtwith the scenario error and, per run, where its logs went or why they couldn't be collected (no repo, no CI driver, no runs, list or fetch errors)workflow-logs-unavailable.txt: a failed log fetch now writes this file with the reason, where the logs would have gone.ci.RunLister(new, optional interface,ListRecentRuns): implemented by the GitHub Actions and GitLab CI drivers on top offorge.Client.ListRecentWorkflowRuns. It is optional so externalci.Driverimplementations keep compiling.Worldfields:ScenarioNameandScenarioBeginare set by the Before hook.SavedLogRunIDsprevents fetching the same logs twice.0600, as behaviour tests: harden credential redaction in saved failure logs and document the failed-run return #7948 requires.behaviour-testing.mdandbehaviour-drivers.mdare updated.if: always(). Its gate on the redact step succeeding is a deliberate no-unredacted-upload rule, so it stays.Testing
make lintpasses (stage changes first, then run). In the sandbox,pre-commitcould not fetch remote hook repos (TLS error), so the applicable hooks were run directly: gofmt, go vet, lint-interface-doc-sync, lint-docs-links, lychee, lint-broken-symlinks, and pre-commit-hooks v6.0.0.golangci-lintv1.64.8 reports only existing findings in files this PR doesn't touch.go test -race ./pkg/behaviourtest/...passes. New unit tests cover:RunListerListRecentRunson both driversChecklist
!for breaking changes)🤖 Generated with Claude Code
Closes #8037
Post-script verification
agent/8037-save-failed-run-logs)5563259490807cf923e5b634b182ab2cbbafef1a..HEAD)