docs: freeze forge pointer and harness snapshot join contract - #57
docs: freeze forge pointer and harness snapshot join contract#57HofniGartner wants to merge 3 commits into
Conversation
GuyZivRH
left a comment
There was a problem hiding this comment.
great direction! do you extend this PR or open a following PR with the rest of the work?
|
Thanks Guy! This PR is intentionally the schema / contract slice only:
Following PRs (separate repos / issues), not stacked here:
Happy to open a follow-up in ABEvalFlow for Phase C observer wiring once the producer + AEH tags/artifact path are in — that should stay out of this PR so the join contract can land first. |
|
Refreshed this branch onto current |
GuyZivRH
left a comment
There was a problem hiding this comment.
PR #57 Consolidated Review — Forge pointer / harness snapshot join contract
PR: #57
Author: HofniGartner
Branch: HofniGartner:feat/join-contract-doc → main (cross-fork)
HEAD: 34ab13f
Diff: +192 / −2 · 4 files · 3 commits
Tickets: APPENG-4985, APPENG-5370, APPENG-5300
CI: no check runs on HEAD
Merge state: BEHIND (~24 commits on main)
Consolidated: 2026-08-06 · prefix mzbqx_
Sources folded in: hvtmq_, xkqre_, wfjkr_, qplmx_, rklwm_
Final verdict
Approve with minor fixes — right contract-first slice; merge after rebase, green CI, and doc↔Provenance field alignment.
One prior pass said Request changes for the same schema mismatch (pipeline_run_url / ref_name). Treat that as a must-fix before merge, not as “block forever” or “wire producers in this PR.” Author already scoped this as schema/contract only (fullsend producer + AEH consumer elsewhere).
What shipped
Freezes a cross-project join schema so three streams can correlate:
| Stream | Artifact | Writer |
|---|---|---|
| Harness snapshot | harness-snapshot.json |
Agent runtime |
| Runtime trace | OTel / MLflow GenAI | Runtime / eval engine |
| Eval verdict | Scorecard / report | ABEvalFlow analyze/store |
| File | Change |
|---|---|
Docs/provenance_forge_pointers.md |
Ownership matrix, field table, lifecycle, MLflow-first read |
abevalflow/report.py |
7 optional join fields on Provenance; eval_engine desc adds aeh |
scripts/analyze.py |
Matching --* CLI flags |
tests/test_analyze.py |
test_provenance_forge_join_fields passthrough |
New fields: repository_url, change_id, trace_id, session_id, eval_run_id, harness_fingerprint, forge_platform.
Consensus strengths
- Correct owner and problem — ABEvalFlow as typed join-record home; “do not invent a parallel schema.”
- Clear ownership + write → handoff → log → read lifecycle; MLflow artifact/tags as canonical read after logging.
- Backward compatible — all new fields optional; existing
commit_sha/pipeline_run_idconsumers stay valid. - OTel-aligned field descriptions (
vcs.*,gen_ai.conversation.id). - Small, reviewable contract slice; producer/consumer wiring intentionally deferred.
- Local provenance tests pass (
pytest … -k provenance→ 4 passed on this HEAD). - Full
Provenancestill lands in Postgres viareport_jsonblob even without new columns.
Must fix before merge
1. Rebase + CI
Branch is stale (~24 commits behind). No GitHub check runs on fork HEAD. Rebase on main, ensure test workflow runs (may need maintainer approval for first-time fork contributor), merge only on green.
2. Doc field table vs typed Provenance
Contract table lists under “Provenance / MLflow tag” but code does not add:
pipeline_run_urlref_name
Pick one:
- A: Add both to
Provenance+ analyze CLI + test, or - B: Mark them snapshot / MLflow-only (not on Provenance), like other snapshot-only identity fields.
A freeze that over-claims typed fields will mislead fullsend/AEH consumers.
3. eval_engine description drift
Description adds aeh but still omits engines already allowed on main (e.g. a2a, and project-wide values such as mcpchecker where applicable). Align string with analyze choices / EvalEngine / observability tables.
Should fix / nice-to-have (non-blocking)
| Item | Notes |
|---|---|
| Test completeness | Assert trace_id / eval_run_id / session_id; optional CLI → report.json round-trip |
| Tekton wiring | Analyze tasks still only pass commit-sha / pipeline-run-id — follow-up |
| Other entry points | aggregate_ase / aggregate_mcpchecker narrower Provenance — OK via defaults |
| MLflowObserver (#64) | Doc lists preferred forge tags; observer does not yet emit full set — follow-up |
| Cross-link #56 | After rebase: link observability_tables.md ↔ forge pointers; note join keys in report_json |
| CRLF on new doc | Normalize to LF if present |
forge_platform |
Optional Literal / enum later |
| Cursor co-author trailers | Prefer not to advertise AI co-authors on future commits |
Related work
| Work | Relationship |
|---|---|
| #56 observability tables | Postgres column map; join keys mostly in report_json today |
| #64 MLflow Observer | Should adopt tag names from this contract |
| #61 AEH MLflow evaluate | Trial-level; separate from fleet forge join keys |
| fullsend / AEH PRs | Producer + MLflow consumer (out of this PR) |
Suggested GitHub action
- Ask author to rebase, fix field-table +
eval_enginewording, push. - Confirm CI green.
- Approve and merge.
- Track follow-ups: Tekton param wiring, observer forge tags, optional hot-join DB columns.
Checklist
- Scope matches stated “contract slice”
- Additive optional fields (backward compatible)
- Local provenance tests pass
- Rebased on current
main - Field table matches typed model (
pipeline_run_url/ref_name) -
eval_enginedescription matches allowed engines - CI green on HEAD
Define ownership matrix, field mapping, and snapshot lifecycle so runtimes and eval engines share one join interface. Co-authored-by: Cursor <cursoragent@cursor.com>
Align typed fields and analyze CLI flags with the frozen join contract in Docs/provenance_forge_pointers.md. Co-authored-by: Cursor <cursoragent@cursor.com>
Address review feedback (GuyZivRH): - Add pipeline_run_url and ref_name to Provenance model, CLI, and tests so the typed model matches the field table in the contract doc. - Update eval_engine description to list all engines allowed on main: harbor, ase, a2a, aeh, mcpchecker, both. - Normalize CRLF → LF on Docs/provenance_forge_pointers.md. Signed-off-by: Hofni Gartner <hgartner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
34ab13f to
c4e22c0
Compare
|
@GuyZivRH — Thanks for the thorough consolidated review. Addressed all must-fix items: ✅ 1. Rebase + CIRebased on current ✅ 2. Doc field table vs typed
|
GuyZivRH
left a comment
There was a problem hiding this comment.
make sure CI is green and rebase
Summary
Freeze the cross-project join contract for harness snapshot / forge pointers, and extend
Provenanceso the typed report matches that field list.Changes
Docs/provenance_forge_pointers.md(ownership matrix, field mapping, snapshot lifecycle, MLflow-first later read)Provenancewith optional join fields (repository_url,change_id,trace_id,session_id,eval_run_id,harness_fingerprint,forge_platform)--*flags inscripts/analyze.pyTest plan
uv run pytest tests/test_analyze.py -k provenance(or project venv equivalent)Docs/provenance_forge_pointers.mdfor field/ownership accuracyRelated
Made with Cursor