feat(stations/notify): wire agent-notify release train - #439
Conversation
Co-Authored-By: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds agent-notify native binaries to release workflows, manifests, provenance and acceptance checks, managed installation and resolution, notification execution, component smoke tests, Windows validation, and selective rollback handling. ChangesAgent-notify release integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Build as build-agent-notify-native
participant Release as assemble-release
participant Manifest as component-manifest generator
participant Acceptance as release acceptance
Build->>Release: provide five platform binaries
Release->>Manifest: generate component manifest and checksums
Manifest->>Acceptance: provide five-component release data
Acceptance->>Acceptance: run asset, health, and version smoke checks
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Greptile SummaryThis PR adds
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (7): Last reviewed commit: "test(ci): pin unpublished source compone..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/publish.yml (1)
170-171: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider
persist-credentials: falseon the new checkout step.Static analysis flags this checkout for leaving the
GITHUB_TOKENin.git/configafter checkout. This job only uploadsrelease-assets/(not the workspace/.gitdir), so the classic artifact-exfiltration vector doesn't directly apply, but hardening is still a cheap win and matches recommended practice for a job that has no need to perform authenticated git operations.🔒️ Suggested fix
- uses: actions/checkout@v5 + with: + persist-credentials: false - uses: actions/setup-go@v5🤖 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/publish.yml around lines 170 - 171, Update the checkout step using actions/checkout@v5 to disable persisted Git credentials by configuring persist-credentials as false; leave the rest of the workflow unchanged.Source: Linters/SAST tools
scripts/windows-native-acceptance.ps1 (2)
541-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: normalize output before
ConvertFrom-Json, for consistency.Other JSON-parsing call sites in this script (
Get-ComponentReport,Assert-OperatorDoctorReady) pipe through(... | Out-String).Trim()beforeConvertFrom-Jsonto guard against multi-line/array output. The new agent-notify smoke check pipes$agentNotifyVersiondirectly, which is fine for the expected single-line case but is inconsistent with the rest of the file.♻️ Suggested tweak
- try { - $agentNotifyPayload = $agentNotifyVersion | ConvertFrom-Json - } catch { + try { + $agentNotifyPayload = ($agentNotifyVersion | Out-String).Trim() | ConvertFrom-Json + } catch {🤖 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 `@scripts/windows-native-acceptance.ps1` around lines 541 - 554, The agent-notify smoke check should normalize the output before JSON parsing for consistency with Get-ComponentReport and Assert-OperatorDoctorReady. Update the $agentNotifyVersion handling before ConvertFrom-Json to aggregate and trim the command output, while preserving the existing malformed-JSON and missing-version validation.
332-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared component-id list to avoid duplication.
The five-component array (
agent-notify,graphtrail,graphtrail-mcp,miseledger,sessionfind) is now duplicated verbatim inAssert-ReleaseManifestAndAssets(line 332) andAssert-ManagedComponentDigests(line 365). A shared script-level constant would prevent drift if a component is added/removed later.♻️ Suggested refactor
+$script:KnownComponentIds = @("agent-notify", "graphtrail", "graphtrail-mcp", "miseledger", "sessionfind") + function Assert-ReleaseManifestAndAssets { ... - foreach ($componentId in @("agent-notify", "graphtrail", "graphtrail-mcp", "miseledger", "sessionfind")) { + foreach ($componentId in $script:KnownComponentIds) {Also applies to: 365-365
🤖 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 `@scripts/windows-native-acceptance.ps1` at line 332, Extract the duplicated five-item component ID array into a shared script-level constant, then update both Assert-ReleaseManifestAndAssets and Assert-ManagedComponentDigests to iterate over that constant instead of defining separate arrays.
🤖 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.
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 170-171: Update the checkout step using actions/checkout@v5 to
disable persisted Git credentials by configuring persist-credentials as false;
leave the rest of the workflow unchanged.
In `@scripts/windows-native-acceptance.ps1`:
- Around line 541-554: The agent-notify smoke check should normalize the output
before JSON parsing for consistency with Get-ComponentReport and
Assert-OperatorDoctorReady. Update the $agentNotifyVersion handling before
ConvertFrom-Json to aggregate and trim the command output, while preserving the
existing malformed-JSON and missing-version validation.
- Line 332: Extract the duplicated five-item component ID array into a shared
script-level constant, then update both Assert-ReleaseManifestAndAssets and
Assert-ManagedComponentDigests to iterate over that constant instead of defining
separate arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b20fa046-9175-43bf-81f6-76da2e741837
⛔ Files ignored due to path filters (1)
docs/component-manifest-v1.schema.jsonis excluded by!docs/**,!**/docs/**
📒 Files selected for processing (25)
.github/workflows/publish.ymlscripts/generate_component_manifest.pyscripts/published-artifact-acceptance.pyscripts/verify_component_manifest_provenance.pyscripts/windows-native-acceptance.ps1src/brigade/add.pysrc/brigade/component_bins.pysrc/brigade/component_install.pysrc/brigade/component_manifest.pysrc/brigade/managed.pysrc/brigade/notifications_cmd.pysrc/brigade/templates/components/manifest-v1.jsontests/component_install_helpers.pytests/test_add.pytests/test_component_bins.pytests/test_component_install.pytests/test_component_manifest.pytests/test_component_manifest_provenance.pytests/test_component_report.pytests/test_doctor.pytests/test_managed.pytests/test_notifications_cmd.pytests/test_publish_workflow.pytests/test_published_artifact_acceptance.pytests/test_release_manifest_generator.py
Co-Authored-By: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
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/test_ci_workflow.py`:
- Around line 536-539: Update the manifest assertions in the test around the
unpublished and published component comprehensions to require that unpublished
equals exactly the set containing only "agent-notify". Preserve the existing
published-component requirement, but ensure any additional component lacking
assets causes the test to fail.
🪄 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: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ecc1af9c-18a0-4cfa-b05d-32804556d3f6
⛔ Files ignored due to path filters (1)
docs/component-manifest-v1.schema.jsonis excluded by!docs/**,!**/docs/**
📒 Files selected for processing (26)
.github/workflows/publish.ymlscripts/generate_component_manifest.pyscripts/published-artifact-acceptance.pyscripts/verify_component_manifest_provenance.pyscripts/windows-native-acceptance.ps1src/brigade/add.pysrc/brigade/component_bins.pysrc/brigade/component_install.pysrc/brigade/component_manifest.pysrc/brigade/managed.pysrc/brigade/notifications_cmd.pysrc/brigade/templates/components/manifest-v1.jsontests/component_install_helpers.pytests/test_add.pytests/test_ci_workflow.pytests/test_component_bins.pytests/test_component_install.pytests/test_component_manifest.pytests/test_component_manifest_provenance.pytests/test_component_report.pytests/test_doctor.pytests/test_managed.pytests/test_notifications_cmd.pytests/test_publish_workflow.pytests/test_published_artifact_acceptance.pytests/test_release_manifest_generator.py
Co-Authored-By: Cursor <cursoragent@cursor.com>
|
Follow-up 82287da fixes the Windows PowerShell 5.1 source-compatibility failure from Actions job 89002397885. Empty asset maps are now detected with an array-wrapped PSObject property count, and the unpublished set is computed before setup and reused for health/smoke gates. PyPI mode remains strict. Captured ./scripts/verify is green in receipt 20260722-175123-work-verify-ac7e9a. |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/windows-native-acceptance.ps1 (2)
430-449: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the component ID set, not only its count.
Five entries can still omit a required component if another ID is duplicated or unexpected. Assert the report contains each expected ID exactly once before applying skippable-status checks.
🤖 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 `@scripts/windows-native-acceptance.ps1` around lines 430 - 449, Update the component validation around $Report.components to verify the expected component IDs are present exactly once, rather than relying only on Count -eq 5. Reject duplicate or unexpected IDs before the existing $Skippable status checks, using the established expected-ID set if available.
476-477: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParse
build_dateas a real UTC timestamp.The regex accepts impossible dates such as
2026-99-99T99:99:99Z, despite this assertion claiming to reject malformed metadata. Use exact date parsing after the nonempty/unknown check.Proposed fix
- if (-not $Payload.build_date -or $Payload.build_date -eq "unknown" -or $Payload.build_date -notmatch '^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$') { + if (-not $Payload.build_date -or $Payload.build_date -eq "unknown") { throw "agent-notify build_date must be a UTC timestamp (YYYY-MM-DDTHH:MM:SSZ), not 'unknown'" } + try { + [DateTime]::ParseExact( + [string]$Payload.build_date, + "yyyy-MM-dd'T'HH:mm:ss'Z'", + [System.Globalization.CultureInfo]::InvariantCulture, + [System.Globalization.DateTimeStyles]::AssumeUniversal + ) | Out-Null + } + catch { + throw "agent-notify build_date must be a valid UTC timestamp (YYYY-MM-DDTHH:MM:SSZ)" + }🤖 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 `@scripts/windows-native-acceptance.ps1` around lines 476 - 477, Update the build_date validation in the agent-notify assertion to perform exact UTC date-time parsing after the existing empty and "unknown" checks, replacing or supplementing the regex-only validation so impossible dates and times are rejected while valid YYYY-MM-DDTHH:MM:SSZ values remain accepted.
🤖 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.
Outside diff comments:
In `@scripts/windows-native-acceptance.ps1`:
- Around line 430-449: Update the component validation around $Report.components
to verify the expected component IDs are present exactly once, rather than
relying only on Count -eq 5. Reject duplicate or unexpected IDs before the
existing $Skippable status checks, using the established expected-ID set if
available.
- Around line 476-477: Update the build_date validation in the agent-notify
assertion to perform exact UTC date-time parsing after the existing empty and
"unknown" checks, replacing or supplementing the regex-only validation so
impossible dates and times are rejected while valid YYYY-MM-DDTHH:MM:SSZ values
remain accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6fe565c-c5d6-433e-8c6f-19f171a7da2e
📒 Files selected for processing (2)
scripts/windows-native-acceptance.ps1tests/test_ci_workflow.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_ci_workflow.py
1 participant
Summary
Why
PR #433 imported the standalone agent-notify history into
stations/notify/. This phase makes the binary part of Brigade's signed component release train and removesgo install ...@latestfrom released installs.Verification
brigade work verify run --target . --command "./scripts/verify" --capture brigade-work20260722-171159-work-verify-3e9324Review focus
Refs #431
Summary by CodeRabbit
New Features
agent-notifyas a managed component with native binaries for Linux, macOS, and Windows.agent-notify.agent-notify version --jsonvalidation during install/health and release acceptance.Bug Fixes
agent-notifyupgrades/removals.Tests