docs(adr): accept ADR 0071 harness snapshot for agent traces - #5524
docs(adr): accept ADR 0071 harness snapshot for agent traces#5524HofniGartner wants to merge 5 commits into
Conversation
Record Level-1 harness-snapshot.json and root-span forge pointers as the run-time join contract; implementation follows in a separate issue. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Accept status, add Options, single Decision, bullet Consequences, and Planned callout for harness-snapshot in the tracing guide (fullsend-ai#5449). Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Cross-ref ADR 0005 in Context, omit trace_id when OTEL_SDK_DISABLED, and add the decided entry to docs/architecture.md Observability. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Keep Observability decided list in sync with the accepted ADR. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by Qododocs(adr): accept ADR 0071 harness snapshot + forge pointer correlation
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
Align architecture.md with the Planned callout in the tracing guide so operators do not treat harness-snapshot.json as already shipped. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the architecture.md inconsistency: the Observability bullet now reads Decided (pending #5449) / "every run will write", matching the Planned callout in the tracing guide. |
waynesun09
left a comment
There was a problem hiding this comment.
Automated verification pass on ADR 0071: numbering, cross-references, and citation accuracy. Findings below as inline comments.
| @@ -0,0 +1,93 @@ | |||
| --- | |||
| title: "71. Harness snapshot and forge pointer correlation for agent traces" | |||
There was a problem hiding this comment.
[HIGH] ADR number 0071 collides with open PR #2791
PR #2791 ("docs: ADR 0071 — Auto-merge", branch adr-0062-auto-merge) is open, mergeable, and independently adds docs/ADRs/0071-auto-merge.md for an unrelated decision. Because ADR filenames embed a distinct slug, the two files don't conflict at the git level, so both PRs can merge cleanly and leave two different documents both titled "71. ..." on main. On main today, docs/ADRs/ jumps directly from 0070-portable-provider-profile-resolution.md to 0073-named-mint-privilege-levels.md — 0071 and 0072 are only free because they're reserved by in-flight PRs (#2791 claims 0071; open PR #5016 claims 0072). This PR's own checklist item "ADR number still free on main at merge (/renumber-adr if needed)" is left unchecked, and a main-only check wouldn't catch this collision, since main alone still shows 0071 as free.
Suggestion: Before merging, check open PRs touching docs/ADRs/ (not just main) for the same slot. Whichever of #5524 / #2791 merges second should renumber at merge time to the actually-free slot — re-verify immediately before merge, since 0072 (#5016) and 0074 (also contested by two other open PRs) are in the same situation.
| - Dispatched child runs must inherit forge context via env so child snapshots | ||
| are complete. | ||
| - GitLab/Bitbucket coverage depends on CI vars or `FULLSEND_*` overrides. | ||
| - Implementation is tracked separately ([#5449](https://github.com/fullsend-ai/fullsend/issues/5449)); |
There was a problem hiding this comment.
[HIGH] Implementation-tracking issue #5449 is closed as not_planned but cited as the active tracker
Issue #5449 ("Implement ADR 0071: harness-snapshot.json + root-span forge pointers") is closed with state reason not_planned, closed the day before this PR was opened, with a comment noting it was "opened too early" (the issue was meant to be filed only after this ADR merges, per the repo's ADR-then-issue workflow — so the closure was correct at the time). However, this PR still ships four live references to #5449 as the active/pending tracker: this ADR's Consequences bullet (here), this ADR's Related section ("Implementation: #5449"), docs/architecture.md ("Decided (pending #5449): ..."), and the callout in docs/guides/infrastructure/distributed-tracing.md ("Implementation is tracked in #5449 ... Until that ships, only run-telemetry.jsonl is guaranteed on disk"). Anyone following any of these links today lands on a closed, "not planned" issue — the opposite of what "pending"/"tracked"/"Planned" are meant to convey.
Suggestion: Reopen #5449 (its closure reason no longer applies once this ADR is Accepted) or file a fresh implementation-tracking issue, and repoint all four references before or immediately upon merge. At minimum, note in the PR description that #5449 will be reopened post-merge.
| local JSON remains the forensic / handoff contract. | ||
| - Dispatched child runs must inherit forge context via env so child snapshots | ||
| are complete. | ||
| - GitLab/Bitbucket coverage depends on CI vars or `FULLSEND_*` overrides. |
There was a problem hiding this comment.
[MEDIUM] "Bitbucket" listed as a partially-covered forge, but it isn't a supported forge
This bullet implies Bitbucket is one of fullsend's targeted forges. ADR 0005 (forge-abstraction-layer, cross-referenced two lines earlier in this ADR's own Context) states "Fullsend must eventually support GitHub, GitLab, and Forgejo" and never mentions Bitbucket. In the codebase, internal/harness/forge_test.go (TestResolveForge_UnknownPlatform) uses "bitbucket" as the canonical example of an invalid forge platform, asserting ResolveForge("bitbucket") errors with "not valid." internal/forge/ on main also only contains github/ and gitlab/ subpackages. This reads as a mix-up with "Forgejo," fullsend's actual third forge, stated as settled fact in an Accepted ADR.
Suggestion: Change "GitLab/Bitbucket" to "GitLab/Forgejo" to match ADR 0005's forge scope and the codebase's forge-validation logic, or reword to "non-GitHub forges" if a generic statement was intended.
|
|
||
| - [ADR 0050](0050-distributed-tracing-instrumentation.md) | ||
| - [ADR 0005](0005-forge-abstraction-layer.md) | ||
| - Shared join contract: [provenance_forge_pointers.md](https://github.com/RHEcosystemAppEng/ABEvalFlow/blob/main/Docs/provenance_forge_pointers.md) |
There was a problem hiding this comment.
[MEDIUM] "Shared join contract" this ADR defers to is not yet merged
Context and Consequences both treat the external "shared join contract" document as an existing, settled artifact this ADR can safely defer field-naming detail to. This Related-section link (https://github.com/RHEcosystemAppEng/ABEvalFlow/blob/main/Docs/provenance_forge_pointers.md) currently 404s — that repo's Docs/ directory on main has no such file. The file exists only in RHEcosystemAppEng/ABEvalFlow#57 ("docs: freeze forge pointer and harness snapshot join contract"), an open, mergeable PR that adds Docs/provenance_forge_pointers.md. So the shared contract is real and in progress, but this ADR cites it as an already-frozen reference via a link that is dead today.
Suggestion: Either hold this ADR until ABEvalFlow#57 merges, link to the PR itself (.../pull/57) instead of the non-existent main path, or inline the minimal field list this ADR actually depends on so the decision doesn't hinge on an external, unmerged document in another org's repo.
Summary
Record ADR 0071: Level-1
harness-snapshot.jsonplus mirrored root-span forge pointers as the run-time join contract. Docs only — implementation remains #5449.Replaces auto-closed #5505 / #5508 after vouch (discussion #5520). Incorporates prior review feedback: Accepted status, Options, single Decision, bullet Consequences, Planned callout, architecture.md sync, ADR 0005 in Context, and clear
OTEL_SDK_DISABLEDbehavior fortrace_id/traceparent.Related Issue
Implementation: #5449
Changes
run-telemetry.jsonl; snapshot under> **Planned:**+ Implement ADR 0071: harness-snapshot.json + root-span forge pointers #5449docs/architecture.md: Observability decided entry linking ADR 0071Testing
mainat merge (/renumber-adrif needed)Checklist
Made with Cursor