Skip to content

[SDTEST-4060] Add Go test automation exclusion declarations - #291

Open
calvinbayer wants to merge 2 commits into
mainfrom
codex/critical-test-annotations-go
Open

calvinbayer wants to merge 2 commits into
mainfrom
codex/critical-test-annotations-go

Conversation

@calvinbayer

@calvinbayer calvinbayer commented Nov 2, 2026 •

Copy link
Copy Markdown

What

Adds per-declaration test automation exclusions to DDTest's native Go Mini instrumentation for Go testing and Testify:

  • //ddtest:prevent_failure_suppression keeps failures blocking despite quarantine or disabling.
  • //ddtest:no_auto_retry excludes the declaration from ATR, EFD and ATF, including retries of enclosing suites.
  • testopt.RegisterTestExclusions provides the initialization-time declaration API used by generated registrations.

The CLI registers annotations from compiler-selected sources, including overlays, cgo files, local/external test packages, imported helpers, aliases, and generic/parenthesized receivers. Runtime execution retains original backend assignments, source attribution and native selection while applying each declaration's effective policy. Mutable span tags do not activate exclusions.

Why

Critical tests need to remain blocking and execute only as permitted by their declaration. The exclusion must survive suite retries and process boundaries without changing unrelated tests, generated cases, native repetitions or framework lifecycle behavior.

E2E testing

Use a mock intake with independently controlled ATR/EFD, management assignments and TIA responses. Run each policy separately, together, and absent:

  1. Declarations and metadata: compile annotated Go tests and Testify pointer/value methods; remove their source before execution. Verify true-only annotation tags and retained disabled/quarantine/ATF assignment tags. Exercise build tags, overlays, active cgo files, external packages, imported/private declarations, pointer aliases, generic instantiations and legal parenthesized receivers. Register public value/pointer method expressions, promoted methods and generic instantiations directly. Compile annotated generic callbacks in ordinary Go sources, including local, external-test and imported helper packages, and invoke concrete single/multiple-parameter instantiations from native tests. Generated registration must compile; child exclusions and first results remain declaration-local while unannotated parents retain ordinary retries. An unrelated declaration or package function with the same method name/signature must retain its own behavior.
  2. Policy and retries: assign disable/quarantine/ATF and enable ATR/EFD. A protected selected failure must block; NoRetry must execute once per native invocation and keep its first result while ordinary siblings recover. Test parent-owned families in both retry modes and supported process descendant families, including first discovery in a later worker. Selected native subtests retain their existing single-execution capability fallback. Omit a previously failing NoRetry child on a later passing parent attempt: its first failed result must still block. Also let EFD pass before first discovering a failing NoRetry child; the family must fail while preserving its earlier passing execution. Keep PFS-only recovery and NoRetry-only quarantine suppression effective, whether that child is omitted or rediscovered. ATF retains any protected failure; ordinary ATR/EFD recovery remains unchanged. Explicit native parent skips/selection boundaries remain effective. First discover a NoRetry method beneath an ordinary wrapper during a process retry of a disabled+ATF root: compare with in-process execution and require the method to stay skipped, its body not to execute, its disabled/ATF assignment tags to remain, and no ATF-completion ownership.
  3. Generated cases and cache identity: use shared callbacks, parallel table rows, duplicate native row names and -count=2. Switch derived suites sharing one base declaration at the same native path: reuse that declaration's first result while preserving reflected source/suite attribution. Explicit overrides and unrelated declarations remain independent, including ancestor switches beneath ordinary process-worker wrappers. Put an annotated callback inside an unannotated Testify method, then create a native row from that callback. Assign disabled/quarantine/ATF to the row and require both inherited exclusion tags, all assignment tags, one failing row execution and a failing command/session (four test events for root, method, callback and row). Switch two annotated parent callbacks using the same native row path/callback: the second declaration must execute and retain its own failing result.
  4. Process transport: discover a NoRetry method with 4,096 rows during a retry worker, then let an ordinary sibling recover. Require all 4,096 callbacks and 4,097 execution events exactly once. Valid event envelopes stay within the existing 16 MiB byte budget; cache/directive limits, per-record identity/status checks and ordinary 64 KiB payload limits remain intact. Budget a 4 KiB log on every row by trimming optional diagnostics while preserving every required record; reject oversized mandatory content, duplicate and unrelated records. Also exercise an excluded declaration that is itself the isolation root. First complete a newly discovered NoRetry declaration in a process worker, then let an ordinary native subtest panic or call Goexit. Require its original event, completed history and failure-protection state to survive early result publication: the excluded callback must not run again or replace its first failure with a later pass. Compare terminal outcomes with native controls, and cover a still-running parallel excluded callback without inventing a completed result. Also first discover a PFS-only native callback during a process retry of a disabled+ATF root, then panic or call bare Goexit. Require the callback terminal to keep the family final result and command failing despite retained root assignments, without adding a NoRetry policy or a new child-reporting capability. Preserve native cleanup cancellation outcomes in separate controls, including a native cleanup SkipNow cancellation that succeeds; an unfinalized raw panic must not turn that cancellation into a protected failure. Conversely, explicitly fail a PFS-only parallel callback, then keep it blocked while an ordinary sibling panics after a readiness handshake. Its already committed protected failure must remain blocking even though that callback has not reached cleanup.
  5. TIA: verify native-supported top-level skipped/forced tests preserve callback counts, annotation/TIA tags and actual session skip counts for both exclusions. A method first discovered in a process worker must retain its sole event's TIA skip/forced metadata. Native in-process Testify retains its existing top-level-only TIA boundary; off/on baseline controls do not introduce a new method-skipping feature.
  6. Native filters: select an ordinary sibling while a filtered-out protected method would fail if invoked. Native, annotation-off and annotation-on controls must all pass without admitting that method. Preserve descendant filters and alternatives through managed subtree and ATF continuation launches.
  7. Native lifecycle: compare native and Mini cleanup failures/panics, parallel Setenv/Chdir guards, sequential-parent cleanup that releases parallel blockers, and direct body terminals. Include cleanup SkipNow/FailNow/Goexit, body-panic replacement and fuzz skip lifecycle. With automatic features disabled, compare go test -count=1 and ddtest instrument go test -count=1 for body plus one/multiple cleanup panics, including a cleanup that registers another cleanup. Check the same diagnostic markers and native exit outcome, with original source attribution. Include cleanup Goexit cancellation and a later cleanup panic. Also run an assigned disabled/quarantined protected test through a Go timeout and a background panic with the CLI's --panic-hook mode. Require one executed failing event with test.final_status=fail, retained assignment tags, and a failing command; a skipped final result is incorrect. Use ordinary and NoRetry-only negative controls: quarantine retains its existing skipped final result; disabled+ATF with NoRetry stays skipped without executing the body. Verify mutable span tags cannot activate protection. Complete a failing PFS-only child under an unannotated quarantined parent, then time out the parent or trigger a background panic with --panic-hook. Also keep a child active after t.Error until timeout or background panic: its normal native Fail hook must protect the active parent before the child finalizes. Both child and parent must retain failed final results and assignment tags; the parent must not gain a synthetic PFS annotation. Cover trusted fact delivery before/after event binding, retry-event resets and concurrent close/crash, while ordinary NoRetry-only or recovered failures keep their existing suppression behavior. Retain native command outcomes and cleanup order; do not admit queued parallel children after an unsuppressed body terminal or release scheduler slots twice.

The CLI helper regressions also run with ordinary startup noise and coverage output before their result. Verify exact dispatch environment state, then kill the probe owner and require its OS lock, session, configuration copy and owned probe to be recovered while the customer test remains intact. Coverage and instrumentation remain enabled.

The executable regression matrix is in ddto/integration/mini_exclusions*_test.go; runner registration units and maintained runtime protocol tests cover declaration and transport boundaries. Reproduce with:

cd ddto
go test -count=1 ./integration -run '^TestMini(DeclarationExclusions|TestifyExclusionsKeepSiblingRetries)$'
PARITY_TEST_MODE=race go test -count=1 ./integration -run '^TestMini(DeclarationExclusions|TestifyExclusionsKeepSiblingRetries)$'
GOTOOLCHAIN=go1.25.11 go test -count=1 ./integration -run '^TestMiniTestifyExclusionsKeepSiblingRetries$/(generic-declarations|promoted-declaration-cache)'
go vet ./...

Automated timings, full compatibility progress and exact-head CI/review results are recorded in the validation comments. No merge or release is included.

@calvinbayer
calvinbayer requested a review from a team as a code owner November 2, 2026 12:12
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@calvinbayer

Copy link
Copy Markdown
Author

Validation at signed head 14c6b92: focused end-to-end exclusion matrix passes (24.5s), including shared parallel table callbacks, declaration isolation, compiled metadata without source, pointer/value Testify methods, cleanup failures, native repetitions, EFD/ATR and both retry modes. go vet, maintained-source verification (397 dd-trace-go files), dependency boundary and all 14 CI-shard checks pass. Full compatibility validation against the repository-pinned Orchestrion and root workflows are running; CI will provide platform validation.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Two critical correctness gaps stand out: exclusion/protection policies are lost for quarantined Testify parents running under -race + process retry mode (findings 0/4), and the DD_CIVISIBILITY_SUBTEST_FEATURES_ENABLED=false feature gate is bypassed when annotation metadata is applied (findings 2/7). Additional distinct issues include promoted-method name mismatches dropping exclusion registrations, unconditional Attempt-to-Fix inheritance overriding explicit child assignments, and annotated methods in imported helper packages never receiving registrations.

Open Bits AI session

🤖 Bits Code Review · Commit 14c6b92

Comment thread ddto/internal/runner/run.go Resolved

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Two of the most critical issues are: (1) in race+process-retry mode, quarantined Testify parent isolation paths skip declaration-exclusion resolution, allowing protected failures to be silently suppressed; and (2) promoted Testify methods (inherited via suite embedding) lose their annotation-based exclusions because the runtime name of the wrapper does not match the registered declaration name.

Open Bits AI session

🤖 Bits Code Review · Commit 14c6b92

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Pipelines  Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 92.41%
• Overall Coverage: 88.64% (+0.01%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: b41d939 | Docs | View more details | Give us feedback!

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

The two most critical issues are: (1) protected-failure state is not accumulated across ATF retry attempts in in-process mode, allowing a command to exit 0 despite an earlier blocking failure; and (2) the exclusion cache uses only t.Name() as its key, enabling cross-suite collisions that can suppress failures or skip unrelated tests during retries. Additional distinct concerns cover cgo-sourced declarations being invisible to exclusion scans, ordinary nested suite methods being incorrectly rejected during process-retry validation, and exclusion payloads exceeding the 64 KiB control-file limit in process transport mode.

Open Bits AI session

🤖 Bits Code Review · Commit 46123b6

Comment thread ddto/internal/runner/exclusions.go Resolved
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head 114e790. All five previous findings are addressed with regressions. Expanded normal, Go 1.25, and final actual race matrices passed (race 109.571s). Policy/protocol units, vet, source integrity, dependency boundaries, CI shard and strict source-frame checks passed. Independent bounded process review returned no findings. Exact-head CI is running.

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Final signed head 48f97bf adds only two per-command signing flags for synthetic fixture commits. Runtime, annotation, policy and transport code is identical to 114e790. Both affected fixture scenarios and all pinned Orchestrion composition variants pass (67.409s). Please review this exact head; final-head CI and the full local Go suite are running.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

The most critical issue is that pointer receiver type aliases (e.g., type P = *Suite) cause the registration code generator in exclusions.go to emit an invalid (*P).TestCritical expression, breaking compilation of otherwise valid instrumented builds. A separate concern is that cleanup panics for annotated top-level tests are silently swallowed rather than re-propagated, diverging from native Go termination behavior.

Open Bits AI session

🤖 Bits Code Review · Commit 48f97bf

Comment thread ddto/internal/runner/exclusions.go Resolved
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head62e6a8339bead34b5c7ace0c96d8b58016631d24. Both prior findings are fixed with native compiler/runtime regressions. Complete normal exclusion matrix54.570s and focused actualrace14.816s pass. Source398integrity, vet, dependency boundary,14shard checks, strictsource-frame controls pass. Independent bounded final delta review: no findings. Exact-head CI and complete local Go suite are running.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

The two most structurally critical issues are: (1) the exclusion/replay cache is re-initialized on every nested management wrapper entry in instrumentation.go, so excluded callbacks re-execute instead of replaying their original result under ATR/EFD/ATF parent retries; and (2) a no_auto_retry Testify method with a full table (4,096 rows) pushes the process-retry cache over its entry limit, blocking unrelated ATR siblings from recovering.

Open Bits AI session

🤖 Bits Code Review · Commit 62e6a83

Comment thread ddto/internal/runner/exclusions.go Resolved
Comment thread ddto/internal/runner/run.go Resolved
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head a98737d. All four prior findings are fixed with real compiler/runtime regressions. Complete normal matrix passed 97.075s, focused corrected nested actual race matrix 53.068s, bounded cache/protocol units 2.116s. Vet, source integrity (398 files), dependency boundary, 14 shard checks, and strict source-frame controls pass. Independent bounded final reads found no further issues. Complete actual race matrix, full local pinned suite, and exact-head CI are running.

@calvinbayer

Copy link
Copy Markdown
Author

Validation update at signed a98737d: the complete final actual race annotation matrix passed (212.075s), alongside the complete normal matrix (97.075s). This includes all four latest review fixes, declaration-isolated nested management counters, native repetitions and duplicate row names, protected cached ancestor outcomes, 4,096 generated rows, and changed ancestors with an independent shared excluded leaf. The full pinned compatibility suite and exact-head CI continue.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Process-isolated descendant retries can rerun excluded tests and let protected failures produce a successful command. Valid isolation-root cache entries are also rejected, and replayed excluded-test events lose TIA metadata.

Open Bits AI session

🤖 Bits Code Review · Commit a98737d

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head 411bc20. All four previous findings are fixed with actual process-boundary regressions. Native filter preservation also fixes a proven selected-sibling regression: native passed, annotation-off passed while admitting the filtered callback, annotation-on failed; all controls now pass without admitting it. Focused final transport/TIA matrix passed 27.759s, native TIA declaration-tag controls 8.712s, selector regression normal 12.595s and actual race 19.970s. Units, vet, source provenance (400 files), dependency boundary and 14 shard checks pass. Independent bounded review found no additional issue. Full normal/race matrices, full pinned local suite and exact-head CI are running on frozen source.

@calvinbayer

Copy link
Copy Markdown
Author

Final frozen-head validation at 411bc20: complete normal annotation matrix passed (155.030s), and complete actual race matrix passed (271.702s). Both include native filter preservation, independent descendant ATF cache/protected outcomes, selected-root cache identity, native top-level TIA declaration tags, and late worker TIA metadata/counters. Bounded units, vet, 400-file provenance, dependency and shard checks pass. Exact-head CI and the full pinned compatibility suite remain in progress.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Adding either exclusion annotation to a generic Testify method causes instrumentation to abort before tests execute. The receiver scanner rejects valid generic receiver syntax.

Open Bits AI session

🤖 Bits Code Review · Commit 411bc20

Comment thread ddto/internal/runner/exclusions.go Resolved
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head ee94b72. The remaining generic receiver finding is fixed, including legal parenthesized receivers. Compiled local/imported pointer/value/multi-parameter fixtures passed on current Go (14.755s), Go 1.25.11 (15.067s) and actual race builds (16.496s); excluded callback/event and ordinary unrelated-declaration controls remain strict. Runner units, vet, unchanged 400-file provenance, dependency boundary and all 14 shard checks pass. Independent bounded review: no findings. Previous signed head had all 146 CI checks green. Final exact-head CI, full normal/race matrices and full pinned local suite are running on frozen source.

@calvinbayer

Copy link
Copy Markdown
Author

Frozen head ee94b72: complete normal annotation matrix passed (120.328s), and complete actual race matrix passed (298.987s), including the final generic and parenthesized receiver regressions. Current/minimum Go compiled generic controls and all required source/dependency/shard checks pass. Exact-head CI and the full pinned compatibility suite continue on unchanged source.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Three distinct correctness issues were found: the protocol envelope for ExcludedTests can be exceeded when a no_auto_retry method is late-discovered during process retries; the parallel-safety flag is prematurely cleared before user cleanup functions run, allowing unsafe process-wide state mutation; and promoted suite methods generate inconsistent cache keys, causing already-excluded declarations to re-execute.

Open Bits AI session

🤖 Bits Code Review · Commit ee94b72

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head 521980e. All three prior findings are fixed with real boundary regressions. Combined final new-delta matrix passed normal 46.493s and actual race 60.449s; lifecycle native guards/ordering/body terminals, promoted declaration/reporting identity, and all 4,097 late-discovered events are preserved. Bounded units, vet, exact source-frame controls, unchanged 400-file provenance and 14 shard checks pass. Independent bounded reviews found no additional issue. PR description now describes final grouped behavior and runnable regression commands. Previous signed head had all 146 CI checks green; final exact-head CI, complete normal/race matrices and full pinned compatibility are running on frozen source.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Three distinct correctness bugs were identified: a receiver-type mismatch that silently bypasses test exclusion lookups for Testify suites, a potential deadlock when cleanup calls SkipNow/FailNow before the scheduler slot is reacquired in parallel subtest scenarios, and a nil exclusion-cache propagation gap that causes excluded descendant tests to run again on subsequent retries. All three fixes also require refreshing the maintained-source hash in SOURCE.json.

Open Bits AI session

🤖 Bits Code Review · Commit 521980e

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Final signed head fe9b50b corrects only the SOURCE.json digest for final ADAPTATIONS.md documentation. Runtime, CLI and test code is identical to 521980e. All-library provenance, dependency boundary and 14 shard checks pass on this head. All three prior review findings remain addressed; combined final regressions passed normal 46.493s and actual race 60.449s, and the full normal annotation matrix passed 119.352s. Exact-head CI, full actual race and full pinned local suite are running unchanged.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Three distinct correctness issues were found in the CI visibility test instrumentation: a cache-key collision that can mask failing rows during in-process retries, a Goexit/panic interaction that aborts remaining package tests for annotated exclusions, and a protection-propagation gap that lets quarantined parents suppress ATF family failures across retry attempts.

Open Bits AI session

🤖 Bits Code Review · Commit fe9b50b

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head 2729cf7d70570132e091d6f1261cb6369e1d347a (PR REST, GraphQL and branch agree). The five actionable review findings are fixed; the independent-ATF masking claim is covered by native fallback and supported process controls, with details in its thread. Native fuzz skip lifecycle is restored.

Final quick validation passed: all maintained-source manifests (400/9/52/19 files), dependency boundary, 14 shard tests, runner/runtime exclusion units, vet, exact source-frame checks, native lifecycle normal/race/Go1.25, public method API normal/race/Go1.25, and cache race controls. Fresh complete annotation matrices and pinned full compatibility suite are running on this frozen source; their results and this head's CI remain pending.

@calvinbayer

Copy link
Copy Markdown
Author

Validation update for 2729cf7: the complete annotation integration matrix passed in normal mode (200.808s) and actual race fixtures (453.638s). Final all-library provenance, dependency boundary, 14 shard tests, runner/runtime exclusion units, vet and strict source-frame controls also passed. Native cleanup regressions passed normal/race/Go1.25.11, including the exact previously failing CI fuzz skip lifecycle. The pinned full compatibility suite and exact-head CI/Datadog review are still running.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Two distinct issues affect NoAutoRetry/excluded-test handling: one concerns serialization budget enforcement for ExcludedTests payloads that can exceed 16 MiB, and the other concerns verdict correctness when a failing excluded declaration is never rediscovered on a passing parent retry attempt, allowing a failed test to be silently suppressed even with PreventFailureSuppression set.

Open Bits AI session

🤖 Bits Code Review · Commit 2729cf7

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Please review signed head 36ac77e386877952f9cbad47b79a32c708584c2b; PR REST, branch and GraphQL heads agree. Both findings in the preceding review are fixed and regression-tested. The existing cleanup helper identity is restored so native fuzz telemetry matches the pinned SDK, without adding stack normalization.

Focused validation passed:28 omitted-child/Q/PFS cases normal11.393s, actualrace50.998s, Go1.25.11 38.875s; bounded mixed/excluded4096-record diagnostic units on current/minimum Go; native fuzz fatal/skip and coverage differential48.027s; strict frame controls; all401/9/52/19 provenance files, dependency boundary,14 shard tests, runner/runtime units and vet. Fresh complete annotation matrices and pinned full compatibility suite are running on this frozen source; exact-head CI remains pending.

@calvinbayer calvinbayer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with an independent reviewer auditing both expectations and raw runtime evidence.
Revision tested: babe0e351a29048d6103d5d5c4672f3fb2bb87e0
Environment: local macOS arm64, Go 1.27.3, Gin playground; DDTest official release builder bundles Mini runtime. All six source checkouts and release binaries match this exact commit, with vcs.modified=false. SDK aa51eff7d37fd57d922594def527d5e070c9c092; Orchestrion 1.14.0.

Shepherd case First run Fresh repeat Assertions per run
Native Go exclusions 20261105-103339.388367000-14431-1 20261105-103521.477775000-18589-1 17/17 PASS
Testify exclusions 20261105-103413.572743000-15749-1 20261105-103553.062742000-19485-1 17/17 PASS
Protected callback in quarantined Testify method 20261105-103233.670008000-10924-1 20261105-103444.131363000-16752-1 15/15 PASS

Result: 98 assertions passed across six runs. The independent reviewer inspected raw Mockdog test/session events, management requests, reports, execution logs and binary metadata. All cases require intentional command exit 1, failed sessions, exact event counts and positive annotation tags; missing events and startup errors fail the checks.

The new callback case runs an actual -race binary in process mode with ATR explicitly disabled. The callback has failure protection alone, retains its backend quarantine assignment, emits once, fails its method/parent/session and command, and never acquires a NoRetry tag. This prevents NoRetry from masking the quarantine regression. Native Go and Testify direct-declaration cases also retain all three backend assignments, both exclusion tags, and no retry/ATF-completion tags. Each case was repeated with fresh process/fixture state.

Method: Crook's go-mini-testing-test-automation-exclusions, go-mini-testify-test-automation-exclusions and go-mini-testify-callback-protection assertions on Gin, using --dep ddtest=codex/critical-test-annotations-go --debug. Crook removes fixtures and restores go.mod/go.sum; generated Orchestrion pin data was preserved. This supersedes the earlier review for e51d13d.

Additional regression checks for this tree: complete normal exclusion matrix 266.054s, focused new regressions with actual race workers 8.629s, minimum Go 1.25.11 actual-race callback matrix 31.624s, runtime units, go vet and maintained-source/dependency checks pass. Full race-matrix and platform CI results are tracked separately. Both latest Datadog findings were reproduced, fixed and replied to; there are no remaining Shepherd-review findings.

Scope: verified real framework execution through Mockdog intake, not the production Datadog UI. The existing in-process selected-quarantine-method capability fallback is preserved; the positive callback case exercises supported race/process isolation. This test-review verdict does not substitute for the separate Datadog code review or platform CI gates.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Three distinct correctness issues were found: Testify nested rows incorrectly inherit the enclosing method's exclusion policy instead of their callback's, process-retry worker wrappers fail to propagate management state to excluded descendants causing skips to be missed, and concurrent body+cleanup panics lose the cleanup panic's diagnostics during finalization.

Open Bits AI session

🤖 Bits Code Review · Commit babe0e3

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@calvinbayer calvinbayer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with an independent reviewer auditing test expectations and raw execution evidence.
Revision: 50925c7f83845e32f46d229cb95322e71ec3d8f3
Environment: local macOS arm64, Go 1.27.3, Gin playground; official DDTest release builder with bundled Mini runtime. All eight source checkouts and tested binaries match this exact commit, vcs.modified=false. SDK aa51eff7d37fd57d922594def527d5e070c9c092; Orchestrion 1.14.0.

Shepherd case First run Fresh repeat Assertions per run
Nested callback row exclusions 20261105-120917.354020000-27339-1 20261105-121048.775356000-32461-1 17/17 PASS
PFS-only callback beneath quarantined method, race/process 20261105-120946.221299000-29833-1 20261105-121106.345404000-33809-1 15/15 PASS
Native Go exclusions 20261105-121009.980650000-30978-1 20261105-121124.958801000-34592-1 17/17 PASS
Testify exclusions 20261105-121029.847503000-31727-1 20261105-121146.539337000-36319-1 17/17 PASS

All 132 assertions passed. The independent reviewer inspected raw parsed test/session events, management HTTP responses, reports, debug logs and build metadata. Each case requires intentional command exit 1 and a failed session, exact event counts, positive exclusion tags, retained backend assignments, and no forbidden retry/ATF-completion tags. The protection-only case uses an actual race/process worker with ATR disabled and requires NoRetry to remain absent. Cases report exactly 4/3/1/2 tests and 2/2/1/2 suites respectively, with one module and session each.

The nested-row case reproduced the previous commit's defect: five assertions failed because the command/session passed and both row exclusion tags were missing. Both runs now fail the command as intended, execute the protected row once and preserve all disabled/quarantine/ATF assignments.

Method: Crook's four go-mini-* assertions on Gin using --dep ddtest=codex/critical-test-annotations-go --debug, each twice. Fresh fixtures/process state, restored go.mod/go.sum and removed fixtures were verified. This supersedes the review of babe0e3.

Related native/worker acceptance checks cover callback-policy inheritance, disabled+ATF inheritance through ordinary worker wrappers, and body/multiple/dynamic cleanup panic diagnostics with native Goexit and queued-child behavior. Focused worker/native/strict-frame/TestMain/manual-RunM checks passed; full actual-race exclusion matrix passed 513.092s, minimum Go 1.25.11 actual-race worker checks 40.697s; vet/lint and all source manifests/inventory checks passed. The separate full local root test command has an identical Mocha guard failure on the published baseline, independently reproduced without source changes; it is not counted as a passing run. Whole-module and platform CI completion are tracked separately.

No remaining Shepherd-review findings. Scope is real instrumented framework execution through Mockdog intake; production Datadog UI was not checked. Ordinary annotation-off event-lifetime differences were compared to the exact published baseline and preserved. Generated Orchestrion pin data and coordinator review artifacts were retained, so no claim is made that the whole workspace is pristine. This verdict does not replace the separate Datadog code review or CI gates.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Protected tests that time out or encounter a panic-hook crash can still emit a skipped final status because crash finalization interprets retained quarantine and disabled assignment tags as effective policy. The command remains failing, but the protected test’s final result is misclassified.

Open Bits AI session

🤖 Bits Code Review · Commit 50925c7

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@calvinbayer calvinbayer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with independent review of expectations and raw runtime evidence.
Revision: 492b5889994a424712a576712130d5a8692d4779
Environment: macOS arm64, Go 1.27.3, Gin playground; official release builder with bundled Mini. Every source checkout and tested binary matches this exact commit, vcs.modified=false. SDK aa51eff7d37fd57d922594def527d5e070c9c092, Orchestrion 1.14.0.

Case First run Fresh repeat Assertions per run
Protected timeout finalization 20261105-124434.988245000-99358-1 20261105-124636.979518000-8105-1 18/18 PASS
Nested callback row exclusions 20261105-124458.214790000-1325-1 20261105-124658.645690000-9170-1 17/17 PASS
PFS-only callback in quarantined method, race/process 20261105-124522.682133000-3707-1 20261105-124717.631267000-10209-1 15/15 PASS
Native Go exclusions 20261105-124549.815414000-5091-1 20261105-124737.882411000-11035-1 17/17 PASS
Testify exclusions 20261105-124613.376567000-7138-1 20261105-124759.682710000-11778-1 17/17 PASS

All 168 assertions passed. The independent reviewer checked raw parsed tests/sessions, management HTTP responses, logs, reports, build identity and fixture restoration. All cases require intentional command exit 1, failed sessions, exact event counts, correct true-only annotation tags, retained backend assignments and no forbidden retry/ATF-completion tags. Missing events or setup errors cannot pass.

The protected timeout test has exactly one event with error.type=timeout, test.status=fail and test.final_status=fail, despite retained disabled/quarantine/ATF assignment tags. The same case on 50925c7 failed exactly one of 18 assertions: its final status was skip. Both new-head repetitions pass. The earlier nested-row before/after regression also remains green, and the PFS-only worker case stays independent of NoRetry.

Method: the five local Shepherd go-mini-* Crook assertions on Gin with PR-branch dependency injection and debug logs, each twice. The timeout preset sets Go's explicit one-second timeout against a five-second body. Crook removed all fixtures and restored module/dependency files. This supersedes the review for 50925c7.

Additional crash-policy acceptance checks passed: 18 normal cases 16.045s, actual race 31.516s, minimum Go 1.25.11 27.655s; normal/race unit controls cover legacy manual fallback, mutable-tag isolation, first-write behavior and concurrent close/crash. Vet, strict source-frame/inventory checks, dependency boundaries and all maintained-source manifests pass. Prior complete actual-race exclusion coverage passed 513.092s; the repository's official ten-group normal validation is running separately on this exact revision.

No unaddressed Shepherd-review findings. Production Datadog UI was not checked; this verifies real instrumented framework execution through Mockdog intake. Platform CI and Datadog code review remain separate gates. Local full-root Mocha and signal-test failures were independently reproduced unchanged on published baselines and are not counted as passing tests. The monolithic local integration command reached its global time budget without failed integration cases; full coverage is being run through the existing official groups without changing inventory or per-group limits.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

A controlled native-subtest panic in a process retry drops completed NoRetry declarations from the worker result, allowing them to execute again and losing their original events.

Open Bits AI session

🤖 Bits Code Review · Commit 492b588

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@calvinbayer calvinbayer left a comment •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with independent review of expectations and raw runtime evidence.
Revision: ccb6c493bf8257000fb08b8d69a6f23e1e6e46aa
Environment: macOS arm64, Go 1.27.3, Gin playground; official release builder with bundled Mini. All 12 source checkouts and tested binaries match this exact signed commit, vcs.modified=false. SDK 65148c8b23d01ab8d21526ce2438b625a326746d, Orchestrion 1.14.0.

Case First run Fresh repeat Assertions per run
Early worker panic / original NoRetry failure 20261105-140138.595728000-4761-1 20261105-140347.613083000-12066-1 15/15 PASS
Protected timeout finalization 20261105-140158.964468000-5958-1 20261105-140403.355553000-12667-1 18/18 PASS
Nested callback row exclusions 20261105-140220.317728000-7623-1 20261105-140420.064841000-13347-1 17/17 PASS
PFS-only callback in quarantined method, race/process 20261105-140243.680442000-9911-1 20261105-140437.476817000-13776-1 15/15 PASS
Native Go exclusions 20261105-140305.516139000-10715-1 20261105-140456.232778000-14420-1 17/17 PASS
Testify exclusions 20261105-140328.266963000-11407-1 20261105-140513.960277000-14918-1 17/17 PASS

All 198 assertions passed across six cases, each repeated from fresh state. Every case requires intentional command exit 1, a failing session, exact event counts and correct annotation tags. The management cases additionally require retained disabled/quarantine/ATF assignments and absence of forbidden retry or ATF-completion tags. Crook itself exits 0 only when the expected product failure and all intake assertions match.

The new early-panic case first discovers and fails a protected NoRetry callback in a process retry, then an ordinary child panics. Both fresh runs retain the exact original error retained protected first failure, its single failing event and final status, both exclusion flags and no retry tags. Independent inspection of the actual stashed counter files confirms three root attempts and one physical excluded callback execution (attempt=xxx, critical=x) in each run. The published 492b588 baseline instead executed it twice (critical=xx), replaced the failure with a pass and falsely returned command exit 0: four of the same 15 assertions failed.

Independent review checked raw parsed tests/sessions, original error metadata, management HTTP responses, reports, debug logs, build identity, stashed counters and fixture restoration. All six local Shepherd go-mini-* Crook assertions used PR-branch dependency injection. Crook removed the fixtures, restored module/dependency files and stashed/removed managed state between repetitions. This supersedes the Shepherd review for 492b588.

Additional worker-terminal acceptance checks passed: 17 Mini cases plus 10 native controls, normal 18.281s, actual race 46.069s, Go 1.25.11 21.163s. Admission-latch concurrency units pass normally and under race; full vet, strict source-frame checks, unchanged integration inventory, all maintained-source manifests, dependency boundaries, 14 shard tests and root lint pass. CLI helper regressions preserve exact environment/ownership checks with coverage and ordinary startup output; quiet/noisy variants and existing version/help checks pass with race and coverage enabled.

No unaddressed Shepherd-review findings. Scope: real instrumented framework execution through Mockdog intake; production Datadog UI was not checked. Exact-head platform CI and Datadog code review are separate gates. Full local root tests retain the previously reproduced unchanged Mocha environment failure and are not counted as passing. The complete two-test annotation matrix also passed with actual race instrumentation on this exact revision in 548.997s, covering native/off/on cleanup provenance, worker terminals, management, cache, EFD/ATR/ATF, payload and capability-fallback controls.

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

A race between terminal result publication and cleanup registration in the process-retry path for disabled+ATF roots causes ProtectedDescendantFailure to remain false, allowing packageFailed to suppress a genuine protected failure and report a spurious success. The fix requires propagating child protection before committing the terminal result.

Open Bits AI session

🤖 Bits Code Review · Commit ccb6c49

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@calvinbayer calvinbayer left a comment •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with independent review of expectations and raw runtime evidence.
Revision: febe7c38fb3a114b4e940010e919500c2af64b72
Environment: macOS arm64, Go 1.27.3, Gin playground; official release builder with bundled Mini. All 18 source checkouts and tested binaries match this exact signed commit, vcs.modified=false. SDK 7ea7c9015443c9b9e00e310bdbcd61f7f33686d0; Orchestrion 1.14.0.

Case First run Fresh repeat Assertions per run
PFS-only native worker terminal 20261105-144310.503989000-72547-1 20261105-144611.315133000-80521-1 24/24 PASS
PFS-only parallel irreversible failure 20261105-144332.757514000-74390-1 20261105-144630.957280000-81073-1 24/24 PASS
Native cleanup cancellation control 20261105-144354.137183000-76417-1 20261105-144647.342285000-81509-1 21/21 PASS
Early panic / original NoRetry failure 20261105-144411.395749000-76933-1 20261105-144702.570404000-81924-1 15/15 PASS
Protected timeout finalization 20261105-144429.174272000-77606-1 20261105-144719.728226000-82480-1 18/18 PASS
Nested callback row exclusions 20261105-144447.650281000-78169-1 20261105-144737.647725000-82931-1 17/17 PASS
PFS-only callback in quarantined method, race/process 20261105-144503.863092000-78626-1 20261105-144753.947946000-83310-1 15/15 PASS
Native Go exclusions 20261105-144524.626665000-79223-1 20261105-144813.079654000-84231-1 17/17 PASS
Testify exclusions 20261105-144548.841728000-79980-1 20261105-144829.515015000-84780-1 17/17 PASS

All 336 assertions passed across nine cases, each repeated from fresh state. Eight positive cases require intentional command exit 1 and failing sessions. The native cleanup cancellation control requires command exit 0 and a passing session. Exact event counts, assignment metadata, applicable annotation tags and forbidden retry/error tags are checked per case. Crook exits 0 only when its expected product outcome and intake assertions match.

The three new worker cases independently cover the latest Datadog finding and its concurrency/cancellation boundaries:

  • Protected panic: a disabled+ATF root discovers a PFS-only callback during a worker retry. Its exact panic remains visible, and final family status and session stay failed despite the root assignments.
  • Irreversible parallel failure: the protected callback returns from t.Error before closing a readiness channel and remaining blocked. An ordinary sibling waits for that channel before panicking. Explicit native -parallel=2 guarantees admission. The already committed protected failure keeps the family blocking even without callback cleanup.
  • Native cancellation: cleanup SkipNow cancels the body panic. The retry is raw PASS/final SKIP, retains disabled/ATF assignments and attempt_to_fix_passed=false, and has no panic error tags. The command/session stay successful. A premature raw-panic protection marker cannot pass this control.

Both repetitions of each new case have exactly two root events and no synthetic child event, preserving the existing native worker reporting capability. Actual stashed counters are attempt=xx, critical=x. None adds a NoRetry annotation. On published ccb6c49, the same positive cases falsely returned command 0/final skip/passing session (three of 24 assertions failed each). The cancellation case kept command success but reported a failed retry and stale testmain_exit_conflict errors (three of 21 assertions failed).

The earlier NoRetry case also retains its original failed callback event/error without re-execution: both actual counter stashes are attempt=xxx, critical=x. The 492b588 baseline instead reran it and replaced its failure with a pass. Protected timeout, callback ownership, true-only exclusion tags, retained management assignments and direct native/Testify cases remain green.

Independent review checked all raw reports, parsed tests/sessions, error metadata, management HTTP responses, debug logs, build identity, counter stashes and fixture restoration. These are nine local Shepherd go-mini-* Crook cases with PR-branch injection. Crook removed fixtures, restored module/dependency files and stashed/removed managed state between runs. This supersedes the Shepherd review for ccb6c49.

Additional focused acceptance checks passed: normal 42.294s, actual race 77.462s, Go 1.25.11 42.856s, including the native seven-terminal oracle, PFS/native/ordinary concurrent controls, queued/self-parallel lifecycle, permitted ATR recovery and the prior history/admission matrix. Vet, strict source-frame checks, complete unchanged integration inventory, source manifests, dependency boundaries, 14 shard tests and root lint pass.

No unaddressed Shepherd-review findings. Scope: real instrumented framework execution through Mockdog test-cycle intake; production Datadog UI and telemetry were not checked. Exact-head platform CI and Datadog code review remain separate gates. Full local root tests retain the independently reproduced unchanged Mocha environment failure and are not counted as passing. The complete two-test annotation matrix also passed with actual race instrumentation on this exact revision in 579.570s, including native lifecycle/cancellation, irreversible concurrent failures, retry history, active admission, management and payload controls.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

The exact base-to-head change has no confirmed reportable regression: the specialist review found no actionable findings, and the human discussion contains no unresolved objection to this revision. Static coverage measured 49 reviewed files and one deliberately skipped generated manifest.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit febe7c3

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

A quarantined parent can still receive test.final_status=skip after a protected child fails if a timeout or background panic interrupts normal finalization.

Open Bits AI session

🤖 Bits Code Review · Commit febe7c3

@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

In in-process execution, a protected child test that calls t.Error and blocks never delivers its failure marker to the quarantined parent before finalization, so failForCrash incorrectly reports the parent's status as skip rather than fail. The fix must propagate the trusted failure to active ancestors at the point the failure is observed, not at finalizer time, since a blocked callback may never reach the finalizer.

Open Bits AI session

🤖 Bits Code Review · Commit 5285ff1

@calvinbayer calvinbayer left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with independent review of expectations and raw runtime evidence.
Revision: 5285ff11d8f04a9c7e89399fb747a020ab81eaf9
Environment: macOS arm64, Go 1.27.3, Gin playground; official release builder with bundled Mini. All 20 source checkouts and tested binaries match this exact signed commit, vcs.modified=false. SDK 7ea7c9015443c9b9e00e310bdbcd61f7f33686d0; Orchestrion 1.14.0.

Case First run Fresh repeat Assertions per run
Quarantined parent crash after protected child 20261105-152827.325258000-27882-1 20261105-153148.369134000-35339-1 24/24 PASS
PFS-only native worker terminal 20261105-152851.657095000-29728-1 20261105-153211.366991000-35930-1 24/24 PASS
PFS-only parallel irreversible failure 20261105-152910.506475000-30555-1 20261105-153230.167456000-36387-1 24/24 PASS
Native cleanup cancellation control 20261105-152929.334445000-31081-1 20261105-153248.864211000-37313-1 21/21 PASS
Early panic / original NoRetry failure 20261105-152951.691997000-31796-1 20261105-153310.845591000-37799-1 15/15 PASS
Protected timeout finalization 20261105-153009.977605000-32398-1 20261105-153329.552506000-38249-1 18/18 PASS
Nested callback row exclusions 20261105-153031.278921000-33162-1 20261105-153347.221568000-39576-1 17/17 PASS
PFS-only callback in quarantined method, race/process 20261105-153050.644166000-33755-1 20261105-153405.611826000-40380-1 15/15 PASS
Native Go exclusions 20261105-153112.461396000-34374-1 20261105-153424.249682000-41183-1 17/17 PASS
Testify exclusions 20261105-153130.324018000-34832-1 20261105-153442.164883000-41773-1 17/17 PASS

All 384 assertions passed across ten cases, each repeated from fresh state. Nine positive cases require intentional command exit 1 and failing sessions. The native cleanup cancellation control requires command exit 0 and a passing session. Exact event counts, assignment metadata, applicable annotation tags and forbidden retry/error tags are checked per case. Crook exits 0 only when its expected product outcome and intake assertions match.

The latest parent-crash case first completes a failing PFS-only child beneath an unannotated quarantined parent, then reaches Go's native timeout. Both actual child and parent events remain FAIL/finalFAIL while retaining Q assignments; the parent does not acquire a synthetic PFS or NoRetry annotation. Actual stashed counters are child=x, after-child=x, proving completion before the crash. Published febe7c3 failed exactly one of the same 24 assertions: its parent final status was SKIP. Normal command failure, child result, assignment tags and timeout metadata already matched. Both fresh new-head repetitions pass.

The three worker cases also cover the prior Datadog finding and its concurrency/cancellation boundaries:

  • Protected panic: a disabled+ATF root discovers a PFS-only callback during a worker retry. Its exact panic remains visible, and final family status and session stay failed despite the root assignments.
  • Irreversible parallel failure: the protected callback returns from t.Error before closing a readiness channel and remaining blocked. An ordinary sibling waits for that channel before panicking. Explicit native -parallel=2 guarantees admission. The already committed protected failure keeps the family blocking even without callback cleanup.
  • Native cancellation: cleanup SkipNow cancels the body panic. The retry is raw PASS/final SKIP, retains disabled/ATF assignments and attempt_to_fix_passed=false, and has no panic error tags. The command/session stay successful. A premature raw-panic protection marker cannot pass this control.

Both repetitions of each of those three worker cases have exactly two root events and no synthetic child event, preserving the existing native worker reporting capability. Actual stashed counters are attempt=xx, critical=x. None adds a NoRetry annotation. On published ccb6c49, the same positive cases falsely returned command 0/final skip/passing session (three of 24 assertions failed each). The cancellation case kept command success but reported a failed retry and stale testmain_exit_conflict errors (three of 21 assertions failed).

The earlier NoRetry case also retains its original failed callback event/error without re-execution: both actual counter stashes are attempt=xxx, critical=x. The 492b588 baseline instead reran it and replaced its failure with a pass. Protected timeout, callback ownership, true-only exclusion tags, retained management assignments and direct native/Testify cases remain green.

Independent review checked all raw reports, parsed tests/sessions, error metadata, management HTTP responses, debug logs, build identity, counter stashes and fixture restoration. These are ten local Shepherd go-mini-* Crook cases with PR-branch injection. Crook removed fixtures, restored module/dependency files and stashed/removed managed state between runs. This supersedes the Shepherd review for febe7c3.

Additional focused acceptance checks passed: normal 55.908s, actual race 67.065s, Go 1.25.11 60.897s, including parent timeout/background-panic and supported retained ATF histories, the trusted event-fact lifecycle and atomic metadata binding, native terminal/cancellation controls, PFS/native/ordinary concurrency, queue/recovery and the prior history/admission matrix. Vet, strict source-frame checks, complete unchanged integration inventory, source manifests, dependency boundaries, 14 shard tests and root lint pass.

No unaddressed Shepherd-review findings. Scope: real instrumented framework execution through Mockdog test-cycle intake; production Datadog UI and telemetry were not checked. Exact-head platform CI and Datadog code review remain separate gates. Full local root tests retain the independently reproduced unchanged Mocha environment failure and are not counted as passing. The complete two-test annotation matrix also passed with actual race instrumentation on this exact revision in 609.732s, including parent crash/ATF history and all earlier native lifecycle, concurrency, retry, cache and payload controls.

@calvinbayer
calvinbayer force-pushed the codex/critical-test-annotations-go branch from 5285ff1 to dabeaf7 Compare November 5, 2026 15:16
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

Targeted runner, runtime, and integration validation supports the declaration-exclusion behavior. The full compatibility matrix remains unverified, and production telemetry was intentionally excluded.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit dabeaf7

@calvinbayer calvinbayer left a comment •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with independent review of expectations and raw runtime evidence.
Revision: dabeaf7ef5a18c663298c0afd028b027032e3d53
Environment: macOS arm64, Go 1.27.3, Gin playground; official release builder with bundled Mini. All 22 source checkouts and tested binaries match this exact signed commit, vcs.modified=false. SDK 1bdbf936737ab9184afd16295e4377feb58e9a52, 7ea7c9015443c9b9e00e310bdbcd61f7f33686d0; injected Orchestrion v1.14.0, with Mini selecting its bundled runtime (orchestrion=false).

Case First run Fresh repeat Assertions per run
Active protected child timeout 20261105-161719.156978000-96778-1 20261105-162138.209971000-7121-1 24/24 PASS
Quarantined parent crash after protected child 20261105-161748.997323000-97897-1 20261105-162158.846224000-7686-1 24/24 PASS
PFS-only native worker terminal 20261105-161811.527108000-98540-1 20261105-162220.100184000-8315-1 24/24 PASS
PFS-only parallel irreversible failure 20261105-161834.057621000-99373-1 20261105-162243.484815000-9407-1 24/24 PASS
Native cleanup cancellation control 20261105-161858.863245000-625-1 20261105-162302.397541000-10026-1 21/21 PASS
Early panic / original NoRetry failure 20261105-161926.658132000-1761-1 20261105-162322.151897000-10726-1 15/15 PASS
Protected timeout finalization 20261105-161946.938250000-2601-1 20261105-162343.805983000-11489-1 18/18 PASS
Nested callback row exclusions 20261105-162008.207459000-3417-1 20261105-162407.558158000-13950-1 17/17 PASS
PFS-only callback in quarantined method, race/process 20261105-162031.184911000-4077-1 20261105-162426.646336000-14569-1 15/15 PASS
Native Go exclusions 20261105-162057.072743000-5658-1 20261105-162455.340804000-16817-1 17/17 PASS
Testify exclusions 20261105-162120.278508000-6702-1 20261105-162519.590051000-17957-1 17/17 PASS

All 432 assertions passed across eleven cases, each repeated from fresh state. Ten positive cases require intentional command exit 1 and failing sessions. The native cleanup cancellation control requires command exit 0 and a passing session. Exact event counts, assignment metadata, applicable annotation tags and forbidden retry/error tags are checked per case. Crook exits 0 only when its expected product outcome and intake assertions match.

The new active-child timeout case records parent entry, child entry and return from t.Error exactly once; after-child is absent. Both open child and parent events remain FAIL/finalFAIL through native timeout. The parent retains Q without acquiring PFS or NoRetry. This exercises failure propagation before child finalization, matching production finding 4481813292.

The earlier parent-crash case first completes a failing PFS-only child beneath an unannotated quarantined parent, then reaches Go's native timeout. Both actual child and parent events remain FAIL/finalFAIL while retaining Q assignments; the parent does not acquire a synthetic PFS or NoRetry annotation. Actual stashed counters are child=x, after-child=x, proving completion before the crash. Published febe7c3 failed exactly one of the same 24 assertions: its parent final status was SKIP. Normal command failure, child result, assignment tags and timeout metadata already matched. Both fresh new-head repetitions pass.

The three worker cases also cover the prior Datadog finding and its concurrency/cancellation boundaries:

  • Protected panic: a disabled+ATF root discovers a PFS-only callback during a worker retry. Its exact panic remains visible, and final family status and session stay failed despite the root assignments.
  • Irreversible parallel failure: the protected callback returns from t.Error before closing a readiness channel and remaining blocked. An ordinary sibling waits for that channel before panicking. Explicit native -parallel=2 guarantees admission. The already committed protected failure keeps the family blocking even without callback cleanup.
  • Native cancellation: cleanup SkipNow cancels the body panic. The retry is raw PASS/final SKIP, retains disabled/ATF assignments and attempt_to_fix_passed=false, and has no panic error tags. The command/session stay successful. A premature raw-panic protection marker cannot pass this control.

Both repetitions of each of those three worker cases have exactly two root events and no synthetic child event, preserving the existing native worker reporting capability. Actual stashed counters are attempt=xx, critical=x. None adds a NoRetry annotation. On published ccb6c49, the same positive cases falsely returned command 0/final skip/passing session (three of 24 assertions failed each). The cancellation case kept command success but reported a failed retry and stale testmain_exit_conflict errors (three of 21 assertions failed).

The earlier NoRetry case also retains its original failed callback event/error without re-execution: both actual counter stashes are attempt=xxx, critical=x. The 492b588 baseline instead reran it and replaced its failure with a pass. Protected timeout, callback ownership, true-only exclusion tags, retained management assignments and direct native/Testify cases remain green.

Independent review checked all raw reports, parsed tests/sessions, error metadata, management HTTP responses, debug logs, build identity, counter stashes and fixture restoration. These are eleven local Shepherd go-mini-* Crook cases with PR-branch injection. Crook removed fixtures, restored module/dependency files and stashed/removed managed state between runs. This supersedes the Shepherd review for 5285ff1.

Additional focused acceptance checks passed: normal 110.093s, actual race 157.361s, Go 1.25.11 116.743s, including parent timeout/background-panic and supported retained ATF histories, the trusted event-fact lifecycle and atomic metadata binding, native terminal/cancellation controls, PFS/native/ordinary concurrency, queue/recovery and the prior history/admission matrix. Vet, strict source-frame checks, complete unchanged integration inventory, source manifests, dependency boundaries, 14 shard tests and root lint pass.

No unaddressed Shepherd-review findings. Scope: real instrumented framework execution through Mockdog test-cycle intake; production Datadog UI and telemetry were not checked. Exact-head platform CI and Datadog code review remain separate gates. Full local root tests retain the independently reproduced unchanged Mocha environment failure and are not counted as passing. The complete two-test annotation race matrix also passed on this exact source in 665.139s (PARITY_TEST_MODE=race).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

Annotated generic callback functions can make the generated instrumentation source fail to compile: the runner emits a direct registration for an uninstantiated generic function instead of using compiled-name registration. The PR discussion addresses generic receiver declarations, but this separate callback path remains uncovered by the added runner tests.

Open Bits AI session

🤖 Bits Code Review · Commit dabeaf7

Comment thread ddto/internal/runner/exclusions.go Resolved
@calvinbayer

Copy link
Copy Markdown
Author

@autotest Review

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

This PR adds declaration-level Go test exclusions for native Go and Testify execution, preserving management assignments, retries, process boundaries, and native lifecycle behavior. The earlier P2 for annotated generic callbacks generating invalid uninstantiated registrations is fixed in this commit with runtime-name registration and internal/integration coverage; no open findings remain. Production-aware validation was not applicable to this static follow-up.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit b41d939

@datadog-datadog-prod-us1 datadog-datadog-prod-us1 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

Focused verification covered declaration-local retry retention and failure protection through native cleanup, with no reportable defects identified. Production behavior and the full compatibility matrix remain unvalidated.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit b41d939

@calvinbayer calvinbayer left a comment •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shepherd E2E test review: PASS

Tested by: Shepherd Agent (autonomous QA for Datadog Test Optimization), with independent review of expectations and raw runtime evidence.
Revision: b41d939f4b3247466a99265059850b45cdbf1a39
Environment: macOS arm64, Go 1.27.3, Gin playground; official release builder with bundled Mini. All 24 source checkouts and tested binaries match this exact signed commit, vcs.modified=false. Shepherd dependency checkout dd-trace-go@1bdbf936737ab9184afd16295e4377feb58e9a52; Mini uses the SDK vendored in this PR (base 870449702d0a0cea26a6223eefe2f0a198069d79, adapted hashes verified); injected Orchestrion v1.14.0, with Mini selecting its bundled runtime (orchestrion=false).

Case First run Fresh repeat Assertions per run
Generic callback declaration exclusions 20261105-164656.611442000-51410-1 20261105-165120.484436000-61475-1 32/32 PASS
Active protected child timeout 20261105-164726.184061000-52905-1 20261105-165143.407949000-62075-1 24/24 PASS
Quarantined parent crash after protected child 20261105-164755.075642000-53919-1 20261105-165207.720802000-62925-1 24/24 PASS
PFS-only native worker terminal 20261105-164819.385561000-54737-1 20261105-165231.512007000-63747-1 24/24 PASS
PFS-only parallel irreversible failure 20261105-164838.665041000-55486-1 20261105-165250.425008000-64398-1 24/24 PASS
Native cleanup cancellation control 20261105-164856.440096000-57111-1 20261105-165309.103329000-65063-1 21/21 PASS
Early panic / original NoRetry failure 20261105-164919.309040000-57958-1 20261105-165331.448178000-65628-1 15/15 PASS
Protected timeout finalization 20261105-164938.673977000-58445-1 20261105-165349.863135000-66358-1 18/18 PASS
Nested callback row exclusions 20261105-164956.460523000-58890-1 20261105-165407.098546000-66796-1 17/17 PASS
PFS-only callback in quarantined method, race/process 20261105-165019.607354000-59690-1 20261105-165425.728433000-67427-1 15/15 PASS
Native Go exclusions 20261105-165043.551529000-60367-1 20261105-165444.549424000-68044-1 17/17 PASS
Testify exclusions 20261105-165102.864818000-60878-1 20261105-165506.990834000-68523-1 17/17 PASS

All 496 assertions passed across twelve cases, each repeated from fresh state. Eleven positive cases require intentional command exit 1 and failing sessions. The native cleanup cancellation control requires command exit 0 and a passing session. Exact event counts, assignment metadata, applicable annotation tags and forbidden retry/error tags are checked per case. Crook exits 0 only when its expected product outcome and intake assertions match.

The new generic callback case places the annotated generic declaration in an ordinary Go source file and invokes its concrete instantiation from a native test in a separate file. Generated registration compiles, and the actual child retains both exclusion tags and disabled/quarantine/ATF assignments without retries. The ordinary parent retains its three failing ATR attempts (two retries), with one final blocking result and no synthetic annotations. Exact counts require four test events and two source suites; actual counter stashes must be parent=xxx, child=x. The corresponding native program compiled and ran before the fix, while the original instrumented build failed on an uninstantiated generated registration (staging review finding 4482397937).

The active-child timeout case records parent entry, child entry and return from t.Error exactly once; after-child is absent. Both open child and parent events remain FAIL/finalFAIL through native timeout. The parent retains Q without acquiring PFS or NoRetry. This exercises failure propagation before child finalization, matching production finding 4481813292.

The earlier parent-crash case first completes a failing PFS-only child beneath an unannotated quarantined parent, then reaches Go's native timeout. Both actual child and parent events remain FAIL/finalFAIL while retaining Q assignments; the parent does not acquire a synthetic PFS or NoRetry annotation. Actual stashed counters are child=x, after-child=x, proving completion before the crash. Published febe7c3 failed exactly one of the same 24 assertions: its parent final status was SKIP. Normal command failure, child result, assignment tags and timeout metadata already matched. Both fresh new-head repetitions pass.

The three worker cases also cover the prior Datadog finding and its concurrency/cancellation boundaries:

  • Protected panic: a disabled+ATF root discovers a PFS-only callback during a worker retry. Its exact panic remains visible, and final family status and session stay failed despite the root assignments.
  • Irreversible parallel failure: the protected callback returns from t.Error before closing a readiness channel and remaining blocked. An ordinary sibling waits for that channel before panicking. Explicit native -parallel=2 guarantees admission. The already committed protected failure keeps the family blocking even without callback cleanup.
  • Native cancellation: cleanup SkipNow cancels the body panic. The retry is raw PASS/final SKIP, retains disabled/ATF assignments and attempt_to_fix_passed=false, and has no panic error tags. The command/session stay successful. A premature raw-panic protection marker cannot pass this control.

Both repetitions of each of those three worker cases have exactly two root events and no synthetic child event, preserving the existing native worker reporting capability. Actual stashed counters are attempt=xx, critical=x. None adds a NoRetry annotation. On published ccb6c49, the same positive cases falsely returned command 0/final skip/passing session (three of 24 assertions failed each). The cancellation case kept command success but reported a failed retry and stale testmain_exit_conflict errors (three of 21 assertions failed).

The earlier NoRetry case also retains its original failed callback event/error without re-execution: both actual counter stashes are attempt=xxx, critical=x. The 492b588 baseline instead reran it and replaced its failure with a pass. Protected timeout, callback ownership, true-only exclusion tags, retained management assignments and direct native/Testify cases remain green.

Independent review checked all raw reports, parsed tests/sessions, error metadata, management HTTP responses, debug logs, build identity, counter stashes and fixture restoration. These are twelve local Shepherd go-mini-* Crook cases with PR-branch injection. Crook removed fixtures, restored module/dependency files and stashed/removed managed state between runs. This supersedes the Shepherd review for dabeaf7.

Additional focused generic-declaration acceptance checks passed: normal 19.950s, actual race 26.558s, Go 1.25.11 20.134s, covering local/external/imported callbacks, multiple type parameters, unused declarations, ordinary callback controls and generic receivers. Vet, strict source-frame checks, unchanged 191-test integration inventory, source manifests, dependency boundaries, 14 shard tests and root lint pass.

No unaddressed Shepherd-review findings. Scope: real instrumented framework execution through Mockdog test-cycle intake; production Datadog UI and telemetry were not checked. Exact-head platform CI and Datadog code review remain separate gates. Full local root tests retain the independently reproduced unchanged Mocha environment failure and are not counted as passing. The complete two-test annotation race matrix also passed on this exact source in 638.450s (PARITY_TEST_MODE=race).

@calvinbayer

Copy link
Copy Markdown
Author

Validation complete for b41d939f4b3247466a99265059850b45cdbf1a39:

  • Root CI and Go instrumentation CI: PASS. All 156 current checks are green. The Windows coverage CLI startup failure cleared on an unchanged-commit rerun; no checks were suppressed.
  • Production Datadog review and staging review: PASS.
  • Independent Shepherd E2E review: PASS, 496 assertions across 24 fresh runs. Complete annotation race matrix: PASS, 638.450s.
  • All 51 review threads resolved; ordinary PR comments also checked for outstanding requests. Commit signature verified; both local worktrees are clean at this revision.

PR remains open and unmerged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant