ci(#6512): fix validate-agents caller permissions and tag the resolved SHA - #6513
Conversation
|
🤖 Finished Review · ✅ Success · Started 3:24 AM UTC · Completed 3:41 AM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (6)ReviewFindingsCritical
Medium
Low
Labels: PR adds CI scripts under .github/scripts/ and modifies Renovate CI configuration Next steps:
Previous run (7)ReviewFindingsMedium
Low
Next steps:
Previous run (8)ReviewFindingsMedium
Low
|
|
/fs-fix Fix the failing tests in CI (but, I think the functional test failure is just a flake, so that one can be ignored). |
|
🤖 Finished Fix · ✅ Success · Started 1:08 PM UTC · Completed 1:18 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.73 |
🔧 Fix agent — iteration 1 (human-triggered)The new .github/scripts/check-agents-gate-pin{,-test}.sh files were not registered in vendoredDefaultsInfraPaths, causing two scaffold tests to fail. Added both paths in alphabetical order. The functional-tests failure was identified by the human as a flake and skipped per instruction. Fixed (1):
Disagreed (1):
Tests: passedNext steps:
|
…paths The check-agents-gate-pin.sh and check-agents-gate-pin-test.sh scripts were added to .github/scripts/ but not registered in vendoredDefaultsInfraPaths, causing TestVendoredDefaultsInfraPathsMatchPredicate and TestEnumerateVendoredPathsMatchesCollectInCheckout to fail. Addresses review feedback on #6513
|
🤖 Review · ❌ Terminated · Started 1:19 PM UTC · Ended 1:34 PM UTC Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 1:19 PM UTC · Completed 1:34 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.11 |
|
🤖 Finished Fix · ✅ Success · Started 1:36 PM UTC · Completed 1:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.84 |
🔧 Fix agent — iteration 2 (bot-triggered)Addressed 3 of 4 review findings with code fixes. Disagreed with 1 (protected-path is an informational/process finding, not an actionable code change). Added a packageRules entry to prevent duplicate Renovate tracking, improved grep error handling to distinguish no-match from file-read errors, and updated the ignore-rule description for accuracy. Fixed (3):
Disagreed (1):
Tests: passedNext steps:
|
- Add packageRules entry disabling native github-actions manager for fullsend-ai/agents to prevent duplicate tracking with the custom regex manager (consumer-completeness finding) - Replace broad `|| true` with exit-code-aware error handling in check-agents-gate-pin.sh grep pipeline (error-handling-idiom finding) - Update ignore-rule description to reflect narrowed scope (scope-description-accuracy finding) Addresses review feedback on #6513
|
🤖 Finished Review · ✅ Success · Started 1:44 PM UTC · Completed 2:04 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.59 |
|
🤖 Finished Fix · ✅ Success · Started 2:06 PM UTC · Completed 2:15 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.20 |
- Write GHA annotations to stdout instead of stderr so Actions UI parses them (pattern-inconsistency finding) - Sanitize RELEASE_YML in annotation strings to prevent :: injection (gha-workflow-command-injection finding) - Detect and reject ambiguous multi-pin configs instead of silently comparing newline-joined SHAs (edge-case finding) - Add multi-pin ambiguity test case - Set GH_TOKEN="fake" in test env to match check-fix-eligibility-test.sh convention (pattern-inconsistency finding) Addresses review feedback on #6513
🔧 Fix agent — iteration 3 (bot-triggered)Addressed 5 of 9 review findings with code fixes: GHA annotation stderr→stdout routing, RELEASE_YML sanitization in annotations, multi-pin ambiguity detection with test, and GH_TOKEN test convention. The critical unauthorized-change finding (Closes→Ref in PR body) was verified correct but requires manual PR body edit — sandbox network policy blocks gh pr edit. Disagreed with 4 findings (design-direction, protected-path acknowledgment, scope-creep, scope-exceeded) with reasoned justifications. Fixed (5):
Disagreed (4):
Tests: passedNext steps:
|
…-tests Two more prefix-rule leaks in the consumer ship-set, same class as the scripts fix: .github/actions/ shipped wholesale. check-e2e-authorization has no consumer outside fullsend's own e2e and functional-tests workflows, and it executes scripts/check-e2e-authorization.sh, which does not ship — consumers received an action that was both unused and broken if invoked. Ship actions by explicit allowlist (the five the vendored reusable workflows execute from ./.defaults/); the re-vendor prune removes the orphan from existing repos. The layered scripts layer shipped its self-tests. WalkLayeredContent and walkLayeredFromRoot had no filter, so pre-fetch-prior-review-test.sh, reconcile-repos-test.sh and resolve-precommit-tools-test.py went out with the layer despite only running in make script-test. Both walks now skip *-test.sh / *-test.py, with a test pinning that the runtime scripts still ship and no test file does. Signed-off-by: Wayne Sun <gsun@redhat.com>
Review found the prune only ran in acquireAndVendor: the combined scaffold+vendor collect func (WorkflowsLayer) and appendVendorTreeFiles (github setup / admin install) committed the new file set without deletes, re-orphaning de-listed files on exactly the install paths most repos use. Move the prune into prepareVendorFiles — the single point every vendor commit path collects its tree — and thread the forge client through VendorCollectFunc and appendVendorTreeFiles to reach it. Also from review: a present-but-invalid vendor manifest now fails the vendor step instead of silently skipping the prune (a missing manifest, i.e. first install, still passes through); the keep-set ignores Delete entries so prune composition stays idempotent; the pin drift check validates the agents main SHA shape with the same 40-hex rule resolve-agents uses; and the pin test header names its real path. TestVendorCommitPathsPruneStaleFiles exercises both real call paths against a seeded manifest and asserts the Delete entries flow through. Signed-off-by: Wayne Sun <gsun@redhat.com>
abdd8c5 to
5a1b639
Compare
|
🤖 Review · Commit: |
…ation Review threads: tag-agents re-validates the resolve-agents output SHA before using it (clear diagnostic if the job-output mechanism ever degrades), and the drift check's mismatch message sanitizes all interpolated values the way the rest of the script does. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 9:50 PM UTC · Completed 10:12 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.87 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
internal/cli/vendor_test.go(file-level): Line 115 · [low] constructor-convention
Pre-existing tests TestAcquireAndVendor_ExplicitPath and TestAcquireAndVendor_CheckoutBuild use &forge.FakeClient{} while the rest of the package uses forge.NewFakeClient(). Not a regression but a missed normalization opportunity.
waynesun09
left a comment
There was a problem hiding this comment.
Maintainer takeover reviewed end-to-end: release permissions fix validated against the v0.37.0 failure evidence, resolve-once tagging, ship-set allowlists with prune verified on all vendor commit paths (Grok review round 2 critical fixed), all threads addressed.
|
🤖 Finished Retro · ✅ Success · Started 11:09 PM UTC · Completed 11:22 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.44 |
Retro: PR #6513 —
|
… tag the gate-validated SHA Preparation for the next release (v0.38.0). Bump validate-agents' reusable-workflow pin from a8566cd5 to agents main b9c07455, which includes agents#1010: the gate now checks out fullsend-ai/agents at an explicit ref instead of github.workflow_sha (the caller's commit), so a cross-repo call can validate for the first time (fullsend#6512 §2, proven on the v0.37.0 run), and it exposes the validated SHA as the agents_sha workflow_call output. tag-agents now tags that gate-validated SHA, falling back to the SHA resolve-agents captured at release start only if the gate output is empty, and logs which source it used. This closes the resolve-once loop end to end: the tree the gate validated is the tree that gets tagged. The pin-drift check stays informational; the checks: read grant that was pre-granted in fullsend-ai#6513 is now exercised by the roll-up job. Signed-off-by: Wayne Sun <gsun@redhat.com>
Every stage job cancels the run working on a work item as soon as a second event arrives for it. The replacement pays sandbox provisioning and bootstrap before the model reads anything, then re-reads the whole item from cold, so a burst of pushes discards finished work and buys nothing: on #6513 six force-pushes produced five completed reviews of commits that were superseded within minutes. FULLSEND_PRESERVE_RUNS lets a repository choose otherwise. Unset — the default everywhere — is exactly today's behaviour. Set to "true", the run in flight finishes and the newer event waits as the single pending run, which then works from the item's current state. The agent's side of that bargain, reconciling current state rather than the state that dispatched it, is fullsend-ai/agents#1163, and is inert until the run facts in the next commit reach the sandbox. Refs #6957 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com> The alignment test decoded cancel-in-progress as a Go bool, which an expression string cannot unmarshal into, so it moves to a yaml.Node and asserts the exact expression every stage job must carry. Signed-off-by: Wayne Sun <gsun@redhat.com>
Every stage job cancels the run working on a work item as soon as a second event arrives for it. The replacement pays sandbox provisioning and bootstrap before the model reads anything, then re-reads the whole item from cold, so a burst of pushes discards finished work and buys nothing: on #6513 six force-pushes produced five completed reviews of commits that were superseded within minutes. FULLSEND_PRESERVE_RUNS lets a repository choose otherwise. Unset — the default everywhere — is exactly today's behaviour. Set to "true", the run in flight finishes and the newer event waits as the single pending run, which then works from the item's current state. The agent's side of that bargain, reconciling current state rather than the state that dispatched it, is fullsend-ai/agents#1163, and is inert until the run facts in the next commit reach the sandbox. Refs #6957 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com> The alignment test decoded cancel-in-progress as a Go bool, which an expression string cannot unmarshal into, so it moves to a yaml.Node and asserts the exact expression every stage job must carry. Signed-off-by: Wayne Sun <gsun@redhat.com>
Every stage job cancels the run working on a work item as soon as a second event arrives for it. The replacement pays sandbox provisioning and bootstrap before the model reads anything, then re-reads the whole item from cold, so a burst of pushes discards finished work and buys nothing: on #6513 six force-pushes produced five completed reviews of commits that were superseded within minutes. FULLSEND_PRESERVE_RUNS lets a repository choose otherwise. Unset — the default everywhere — is exactly today's behaviour. Set to "true", the run in flight finishes and the newer event waits as the single pending run, which then works from the item's current state. The agent's side of that bargain, reconciling current state rather than the state that dispatched it, is fullsend-ai/agents#1163, and is inert until the run facts in the next commit reach the sandbox. Refs #6957 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com> The alignment test decoded cancel-in-progress as a Go bool, which an expression string cannot unmarshal into, so it moves to a yaml.Node and asserts the exact expression every stage job must carry. Signed-off-by: Wayne Sun <gsun@redhat.com>
Every stage job cancels the run working on a work item as soon as a second event arrives for it. The replacement pays sandbox provisioning and bootstrap before the model reads anything, then re-reads the whole item from cold, so a burst of pushes discards finished work and buys nothing: on #6513 six force-pushes produced five completed reviews of commits that were superseded within minutes. FULLSEND_PRESERVE_RUNS lets a repository choose otherwise. Unset — the default everywhere — is exactly today's behaviour. Set to "true", the run in flight finishes and the newer event waits as the single pending run, which then works from the item's current state. The agent's side of that bargain, reconciling current state rather than the state that dispatched it, is fullsend-ai/agents#1163, and is inert until the run facts in the next commit reach the sandbox. Refs #6957 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com> The alignment test decoded cancel-in-progress as a Go bool, which an expression string cannot unmarshal into, so it moves to a yaml.Node and asserts the exact expression every stage job must carry. Signed-off-by: Wayne Sun <gsun@redhat.com>
Summary
Maintainer takeover of the agent-authored PR. The original scope (Renovate tracking of the agents gate pin) implemented an instruction that #6512 has since rejected; this PR now carries the actual release fixes, which need
workflowspermission the code agent doesn't have.Fixes the two
release.ymldefects behind the failedv0.37.0release (startup_failure, run 32615313246) and keeps the drift check as a live, non-blocking release signal.Changes
release.ymlvalidate-agents: grantpull-requests: write+checks: readand document the called workflow's permission contract. GitHub validates a called workflow at parse time and callers can only be downgraded — the missingpull-requests: write(declared by the gate'spull_request_target-onlygatejob) failed the whole run before any job started.checks: readis pre-granted so a future pin bump to agents main (whosefunctional-tests-completedeclares it) cannot reintroduce the startup failure.resolve-agentsjob resolves agentsmainexactly once at release start;tag-agentsnow tags that SHA instead of re-resolvingmainat tag time. Closes the fullsend-side resolve-twice race — anything merged into agents mid-release can no longer be tagged unvalidated.resolve-agentsascontinue-on-error— a stale gate pin annotates every release loudly instead of failing it (the pin bump itself stays a maintainer decision, release: validate-agents startup failure (caller permissions) + agents gate validates a different tree than tag-agents tags #6512).Drift check (
scripts/check-agents-gate-pin.sh+ test, wired intomake script-test).github/scripts/→scripts/: everything under.github/scripts/is vendored into downstream--vendorinstalls and fetched into every agent-run workspace, and this fullsend-release-specific check can never work in a consumer repo. Thevendormanifest.goregistration is reverted accordingly.ahead_by: 0).Reverted
renovate.jsonchanges: the customManager re-implemented the lockstep design release: validate-agents startup failure (caller permissions) + agents gate validates a different tree than tag-agents tags #6512 rejects, and narrowing thefullsend-ai/ignore rule would newly expose the pin to the nativegithub-actionsmanager (which does scan root workflow files — verified via renovate-bot commits torunner-image.yml/lint.ymlafter the scaffoldmanagerFilePatternswas added).Vendored
.github/scripts/ship-set fixed (allowlist + prune)Auditing the vendoring side effect surfaced a broader defect:
isVendoredDefaultsInfrashipped everything under.github/scripts/to consumer repos by path prefix, so repo-local CI tooling entered the user ship-set silently (redact-behaviour-artifacts.sh+ both*-test.shfiles were already shipping; the drift check nearly followed).vendorcontent.go): only the four scripts users actually execute ship —check-fix-eligibility.sh(run by reusable-fix/dispatch) andinstall-podman.sh/install-openshell.sh/openshell-version.sh(run by the root composite action, invoked as./.defaults/from every reusable workflow). The list==walk invariant test now enforces the allowlist instead of transcribing the directory. All four are listed because consumer repos execute them; separately,openshell-version.sh/install-openshell.shcannot move from their paths regardless, because the agents functional-tests gate,hack/gitlab-runner-vm, and the renovate updater read them from a fullsend checkout (path contract, documented in the code).redact-behaviour-artifacts.sh+ test toscripts/(consumers: fullsend's owne2e.yml+ Makefile only); updated the base-branch sparse checkout, relevance regex,paths:trigger and docs.prepareVendorFiles— the chokepoint every vendor commit path collects through, per Grok review of this PR:acquireAndVendor, the combined scaffold+vendor collect func, andappendVendorTreeFilesall receive the delete entries): previously, files recorded in the old vendor manifest but absent from the new set became untracked orphans in consumer repos — the overwritten manifest stopped tracking them, so even uninstall couldn't remove them. Re-vendoring now diffs the committed manifest against the new set and deletes de-listed files in the same commit, so existing vendored repos self-clean on their next CLI bump. A present-but-invalid manifest fails the vendor step instead of silently orphaning; a missing manifest (first install) passes through..github/actions/shipped wholesale.check-e2e-authorizationhas no consumer outside fullsend's owne2e.yml/functional-tests.ymland executesscripts/check-e2e-authorization.sh, which does not ship — consumers received an unused, broken-if-invoked action. Actions now ship by explicit allowlist (the five the vendored reusable workflows execute from./.defaults/); the re-vendor prune removes the orphan from existing repos.scripts/layer shipped its self-tests (pre-fetch-prior-review-test.sh,reconcile-repos-test.sh,resolve-precommit-tools-test.py— referenced only bymake script-test). Both layered walks now skip*-test.sh/*-test.py, with a test pinning that runtime scripts still ship and no test file does.action.yml(references only allowlisted scripts), every path the shipped surface references resolves to shipping content, the layered runtime scripts are all consumer-executed by design, the GitLab scaffold has no scripts, andhack/gitlab-runner-vm(GitLab-forge runner provisioning) copies its two scripts from a checkout with sha256 verification, independent of vendoring..github/scripts/wholesale into the run workspace) is a YAML path pattern and stays prefix-based — relocation, not the Go allowlist, is what removes files from that channel.Out of scope (agents-repo side, tracked in #6512)
Gate cross-repo checkout ref,
workflow_calloutput of the validated SHA, and the roll-up job's cross-repo checkout — after those land, the pin can move to@mainandtag-agentscan tag the gate-validated SHA.Test plan
bash scripts/check-agents-gate-pin-test.sh— 6/6 passgo test ./internal/scaffold/— vendormanifest invariant holds after revertactionlintclean onrelease.ymlande2e.ymlgo test ./internal/scaffold/ ./internal/cli/ ./internal/layers/— allowlist invariant + newStaleVendoredPathstest passbash scripts/redact-behaviour-artifacts-test.shpasses from the new locationTestIsVendoredDefaultsInfracovers the actions allowlist;TestWalkLayeredContent_ExcludesTestFilespins the layered filterTestAppendStaleVendoredDeletes+TestVendorCommitPathsPruneStaleFiles): Delete entries flow through both real commit paths (combined collect func,appendVendorTreeFiles); missing manifest passes through, invalid manifest errorsvalidate-agentsstarts (no startup_failure);resolve-agentsandtag-agentslog the same agents SHA