Retain LangSmith fleet rollup artifact - #2424
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughThe workflow gains three additions to artifact publication: the combined fleet NDJSON is appended to the diagnostics artifact path list, a new always-run "Upload fleet rollup artifact" step publishes it as a standalone artifact with 90-day retention, and its path is added to the report artifact bundle. The test extends existing assertions to verify both the artifact name and path are present. ChangesFleet NDJSON Artifact Publication
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OSV Scanner (2.3.8)Error: ENOENT: no such file or directory, scandir '/inmem/1293/nsjail-0bbb189f-5b67-4994-88ee-047b38e98ccd/merged/.git/hooks' Comment |
Workflow source detectedPR #2424 now has valid workflow source context (origin=local_request). No linked GitHub issue is required for this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9bfaf95a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: langsmith-fleet-rollup-${{ github.run_id }} | ||
| path: .metrics-tmp/fleet/combined-fleet.ndjson |
There was a problem hiding this comment.
For the new langsmith-fleet-rollup-* artifact, the only uploaded path is under .metrics-tmp, and actions/upload-artifact@v7 treats files inside dot-prefixed directories as hidden and excludes them unless include-hidden-files: true is set. Since this step also uses if-no-files-found: ignore, dashboard runs will pass while silently not creating the durable rollup artifact that downstream ingestion is supposed to consume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/maint-80-langsmith-metrics-dashboard.yml:
- Around line 311-318: The upload-artifact action uses an unpinned tag reference
(v7) instead of a SHA-pinned commit hash, which poses a security risk across all
fleet-wide deployments. Replace `actions/upload-artifact@v7` with a SHA-pinned
version of the same action (e.g., `actions/upload-artifact@<commit-sha>`) to
ensure a specific, immutable version is used and prevent unintended changes from
action updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5aba20f2-c314-4aef-8928-6fea8e3ca3f1
📒 Files selected for processing (2)
.github/workflows/maint-80-langsmith-metrics-dashboard.ymltests/workflows/test_langsmith_metrics_dashboard.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
stranske/Template(auto-detected)
e9bfaf9 to
2d2acb4
Compare
Automated Status SummaryHead SHA: 1669e8a
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/maint-80-langsmith-metrics-dashboard.yml (1)
294-297:⚠️ Potential issue | 🟠 MajorPin
actions/upload-artifactby SHA in all upload steps, not just the new one.
Upload dashboard diagnostics(line 296) andUpload report artifact(line 324) still referenceactions/upload-artifact@v7(mutable tag). This keeps a fleet-wide supply-chain risk in a synced workflow.🔒 Minimal fix
- name: Upload dashboard diagnostics if: always() - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with:- name: Upload report artifact if: env.REPORT_AVAILABLE == 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with:As per coding guidelines, flag unpinned third-party actions — this workflow YAML is synced to 9 consumer repos, so one bug replicates fleet-wide.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/maint-80-langsmith-metrics-dashboard.yml around lines 294 - 297, Replace the mutable tag references for the upload-artifact action with pinned SHA hashes to mitigate supply-chain risk. In both the "Upload dashboard diagnostics" step and the "Upload report artifact" step, change `actions/upload-artifact@v7` to `actions/upload-artifact@` followed by the specific commit SHA for that version. This ensures the workflow cannot be compromised by mutations to the v7 tag and prevents fleet-wide replication of any potential vulnerabilities across all synced consumer repositories.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/workflows/test_langsmith_metrics_dashboard.py`:
- Around line 74-79: The test validates that the new rollup step uses a
SHA-pinned upload-artifact action, but lacks a negative assertion to prevent
unpinned upload-artifact@v7 references elsewhere in the workflow. Add an
assertion that ensures unpinned upload-artifact@v7 (without the SHA hash) does
not appear anywhere in the source, which will catch any regressions where other
upload-artifact steps lack proper SHA pinning and prevent this policy from being
bypassed in the synced consumer repositories.
---
Outside diff comments:
In @.github/workflows/maint-80-langsmith-metrics-dashboard.yml:
- Around line 294-297: Replace the mutable tag references for the
upload-artifact action with pinned SHA hashes to mitigate supply-chain risk. In
both the "Upload dashboard diagnostics" step and the "Upload report artifact"
step, change `actions/upload-artifact@v7` to `actions/upload-artifact@` followed
by the specific commit SHA for that version. This ensures the workflow cannot be
compromised by mutations to the v7 tag and prevents fleet-wide replication of
any potential vulnerabilities across all synced consumer repositories.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c37518ef-752a-4935-97ad-26e4fbf46193
📒 Files selected for processing (2)
.github/workflows/maint-80-langsmith-metrics-dashboard.ymltests/workflows/test_langsmith_metrics_dashboard.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
stranske/Template(auto-detected)
2d2acb4 to
4d6bc4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/workflows/test_langsmith_metrics_dashboard.py`:
- Around line 74-80: The assertion on line 80 that checks for
".metrics-tmp/fleet/combined-fleet.ndjson" is redundant with the assertion on
line 76 that already verifies the full path string "path:
.metrics-tmp/fleet/combined-fleet.ndjson" is present in the source. Remove the
redundant assertion on line 80 to reduce test noise and improve clarity, unless
the explicit intent is to verify the path appears in multiple contexts (in which
case, replace it with a count-based assertion to make that intent clear).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bab2d7fa-c8f4-4e79-8ec8-60891bf4e522
📒 Files selected for processing (2)
.github/workflows/maint-80-langsmith-metrics-dashboard.ymltests/workflows/test_langsmith_metrics_dashboard.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
stranske/Template(auto-detected)
| # The raw combined NDJSON is retained for downstream durable ingestion. | ||
| assert "name: langsmith-fleet-rollup-${{ github.run_id }}" in source | ||
| assert "path: .metrics-tmp/fleet/combined-fleet.ndjson" in source | ||
| assert "actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7" in source | ||
| assert "actions/upload-artifact@v7" not in source | ||
| assert source.count("include-hidden-files: true") >= 3 | ||
| assert ".metrics-tmp/fleet/combined-fleet.ndjson" in source |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider removing redundant path assertion.
Line 76 already asserts that path: .metrics-tmp/fleet/combined-fleet.ndjson is present in the workflow source. Line 80 repeats this check by asserting the same path string is present. While this might be intentionally testing that the path appears in multiple contexts (artifact upload steps), the redundancy reduces test signal-to-noise.
If the intent is to verify the path appears in multiple upload steps, consider making this explicit with a count assertion or targeted context checks. Otherwise, remove line 80.
♻️ Optional: Remove redundant assertion
assert "actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7" in source
assert "actions/upload-artifact@v7" not in source
assert source.count("include-hidden-files: true") >= 3
- assert ".metrics-tmp/fleet/combined-fleet.ndjson" in sourceAlternatively, if verifying multiple occurrences is intentional:
- assert ".metrics-tmp/fleet/combined-fleet.ndjson" in source
+ # Path appears in diagnostics, rollup, and report artifacts (3 times)
+ assert source.count(".metrics-tmp/fleet/combined-fleet.ndjson") >= 3📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The raw combined NDJSON is retained for downstream durable ingestion. | |
| assert "name: langsmith-fleet-rollup-${{ github.run_id }}" in source | |
| assert "path: .metrics-tmp/fleet/combined-fleet.ndjson" in source | |
| assert "actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7" in source | |
| assert "actions/upload-artifact@v7" not in source | |
| assert source.count("include-hidden-files: true") >= 3 | |
| assert ".metrics-tmp/fleet/combined-fleet.ndjson" in source | |
| # The raw combined NDJSON is retained for downstream durable ingestion. | |
| assert "name: langsmith-fleet-rollup-${{ github.run_id }}" in source | |
| assert "path: .metrics-tmp/fleet/combined-fleet.ndjson" in source | |
| assert "actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7" in source | |
| assert "actions/upload-artifact@v7" not in source | |
| assert source.count("include-hidden-files: true") >= 3 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/workflows/test_langsmith_metrics_dashboard.py` around lines 74 - 80,
The assertion on line 80 that checks for
".metrics-tmp/fleet/combined-fleet.ndjson" is redundant with the assertion on
line 76 that already verifies the full path string "path:
.metrics-tmp/fleet/combined-fleet.ndjson" is present in the source. Remove the
redundant assertion on line 80 to reduce test noise and improve clarity, unless
the explicit intent is to verify the path appears in multiple contexts (in which
case, replace it with a count-based assertion to make that intent clear).
4d6bc4e to
a18ba5b
Compare
|
Post-push follow-up complete for a18ba5b. Addressed the hidden .metrics-tmp artifact upload issue with include-hidden-files, pinned all upload-artifact uses in this workflow to the existing v7 SHA, replaced the redundant path assertion with an explicit multi-bundle count assertion, and added the workflow-source marker. Local validation passed: uv run pytest tests/workflows/test_langsmith_metrics_dashboard.py tests/scripts/test_langsmith_fleet.py tests/scripts/test_langsmith_fleet_conformance.py; uv run pytest tests/workflows/test_workflow_naming.py; git diff --check. Final PR checks are green and CodeRabbit reported no actionable comments in the latest review. |
Summary
.metrics-tmp/fleet/combined-fleet.ndjsondashboard rollup as a distinctlangsmith-fleet-rollup-<run_id>artifactValidation
uv run pytest tests/workflows/test_langsmith_metrics_dashboard.py tests/scripts/test_langsmith_fleet.py tests/scripts/test_langsmith_fleet_conformance.pyuv run pytest tests/workflows/test_workflow_naming.pyNotes
The artifact name is intentionally distinct from per-repo
langsmith-fleet.ndjsonproducer artifacts to avoid double-counting when individual registered repos start publishing their own fleet records. The local Orchestrator fetcher has been updated separately to use this rollup only as a fallback when no direct per-repo artifacts are available.Summary by CodeRabbit
Release Notes
New Features
Chores
Tests