docs(#4071): broaden workflow sync guidance to cover secret/input threading - #4075
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:46 AM UTC · Completed 7:59 AM UTC |
ReviewFindingsMedium
Labels: PR modifies AGENTS.md documentation about workflow contracts and dispatch patterns |
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 7:28 AM UTC · Completed 7:32 AM UTC |
🔧 Fix agent — iteration 1 (human-triggered)Disagreed with both review findings. The substantive finding (per-repo chain inlining) is factually incorrect: reusable-dispatch.yml currently calls reusable-.yml via uses: references (6 matches), matching what the PR documents. ADR 62 decided to inline stages but this has not been implemented yet. The protected-path finding is procedural and already handled by labels. No code changes needed. Disagreed (2):
Tests: passed Updated by fullsend fix agent |
waynesun09
left a comment
There was a problem hiding this comment.
Automated review-squad pass (3 independent reviewer agents + manual verification against the actual workflow YAML, ADRs 0044/0062, and existing test coverage in this repo). All findings below were independently confirmed against real files on this branch before posting; nothing below was hallucinated or based on pre-existing (out-of-diff) issues alone. One MEDIUM finding from a prior review round (the per-repo chain being "already inlined") was re-checked and confirmed still correctly refuted — not re-raised here.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 3:05 PM UTC · Completed 3:13 PM UTC |
Update the per-repo chain bullet to reference ADR 62, which will inline stages into reusable-dispatch.yml and collapse the two-hop forwarding chain to a single hop. This makes the guidance forward-looking so reviewers know secret threading will change. Addresses review feedback on #4075
🔧 Fix agent — iteration 1 (human-triggered)Addressed 1 of 2 review findings. Updated the per-repo chain bullet in AGENTS.md to reference ADR 62, which will eventually inline stages into reusable-dispatch.yml. The review agent incorrectly claimed stages were already inlined, but the spirit of making the guidance forward-looking is valid. Disagreed with the protected-path finding (procedural, not actionable). Fixed (1):
Disagreed (1):
Tests: passed Decision points
Updated by fullsend fix agent |
Site previewPreview: https://aa0bb7bd-site.fullsend-ai.workers.dev Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Recheck pass on the current head (1c0cb7b) against the six findings from the prior review-squad pass (posted against 21052ce).
Status of prior findings:
- 5 of 6 are unchanged in the current head and remain open — not re-posted here to avoid duplicating the existing threads (AGENTS.md:34 "or variables" claim; AGENTS.md:36 per-org chain / ADR 44; AGENTS.md:34 missing
env:consumption reminder; AGENTS.md:39 "silent empty string" overgeneralization; AGENTS.md:41 missing anti-over-forwarding/test-coverage reminder). - The 6th (AGENTS.md:37, per-repo chain / ADR 62) got a real edit, but the edit introduced a new inaccuracy rather than resolving the original one — reopened and detailed in the inline comment below.
One new finding below (HIGH), independently corroborated by 4 reviewer agents (claude-coder, claude-researcher, grok-review-agent, cursor-code-review) against current main, plus a direct branch-divergence check.
|
/fs-fix rebase and move the guidance to its appropriate place, also read feedback and apply it. |
|
🤖 Finished Fix · ✅ Success · Started 6:31 AM UTC · Completed 6:39 AM UTC |
🔧 Fix agent — iteration 2 (human-triggered)Rebased onto main (507 commits behind) and moved workflow contracts guidance to docs/contributing/workflow-contracts.md, matching the restructured AGENTS.md pattern. Addressed all 7 review findings: corrected the vars/secrets claim, updated per-repo chain to reflect ADR 62 inlining, added ADR 44 deprecation to per-org chain, clarified required-flag consistency for silent failures, added env: injection-prevention guidance, warned against secrets: inherit, and referenced existing automated tests. Fixed (7):
Tests: passed
Decision points
Updated by fullsend fix agent |
Rebase onto main and move workflow contracts guidance to docs/contributing/workflow-contracts.md (the appropriate location after the AGENTS.md restructuring). Addresses all review findings: - Drop "or variables" claim — GHA vars are auto-inherited, only secrets need explicit forwarding - Per-repo chain now reflects the already-inlined architecture per ADR 62 (no separate reusable-<agent>.yml hop) - Per-org chain includes dispatch.yml routing hop and ADR 44 deprecation - Silent-failure guidance clarified with required-flag consistency nuance and FULLSEND_GCP_WIF_PROVIDER example - Added env: consumption security reminder (injection prevention) - Anti-over-forwarding caveat warns against secrets: inherit - References existing automated tests (TestWorkflowCallInputAlignment, TestOTELHeadersSecretThreading) Replaces inline dispatch paragraph in go-code.md with cross-reference. Addresses review feedback on #4075
1c0cb7b to
ed1ad64
Compare
|
/fs-fix rebase and resolve conflicts |
|
🤖 Finished Fix · ✅ Success · Started 12:58 PM UTC · Completed 1:03 PM UTC |
Rebase onto main and move workflow contracts guidance to docs/contributing/workflow-contracts.md (the appropriate location after the AGENTS.md restructuring). Addresses all review findings: - Drop "or variables" claim — GHA vars are auto-inherited, only secrets need explicit forwarding - Per-repo chain now reflects the already-inlined architecture per ADR 62 (no separate reusable-<agent>.yml hop) - Per-org chain includes dispatch.yml routing hop and ADR 44 deprecation - Silent-failure guidance clarified with required-flag consistency nuance and FULLSEND_GCP_WIF_PROVIDER example - Added env: consumption security reminder (injection prevention) - Anti-over-forwarding caveat warns against secrets: inherit - References existing automated tests (TestWorkflowCallInputAlignment, TestOTELHeadersSecretThreading) Replaces inline dispatch paragraph in go-code.md with cross-reference. Addresses review feedback on #4075
🔧 Fix agent — iteration 2 (human-triggered)Rebased onto latest main and resolved AGENTS.md merge conflict. The conflict was between new table rows added on main ([Behaviour Testing], [CI Workflows]) and this branch's [Workflow Contracts] row. All rows are preserved in the resolved file. Markdown link linter passes. Branch needs force-push. Fixed (1):
Tests: passed Updated by fullsend fix agent |
ed1ad64 to
116be14
Compare
|
/fs-fix rebase |
|
🤖 Finished Fix · ✅ Success · Started 9:45 AM UTC · Completed 9:53 AM UTC |
Rebase onto main and move workflow contracts guidance to docs/contributing/workflow-contracts.md (the appropriate location after the AGENTS.md restructuring). Addresses all review findings: - Drop "or variables" claim — GHA vars are auto-inherited, only secrets need explicit forwarding - Per-repo chain now reflects the already-inlined architecture per ADR 62 (no separate reusable-<agent>.yml hop) - Per-org chain includes dispatch.yml routing hop and ADR 44 deprecation - Silent-failure guidance clarified with required-flag consistency nuance and FULLSEND_GCP_WIF_PROVIDER example - Added env: consumption security reminder (injection prevention) - Anti-over-forwarding caveat warns against secrets: inherit - References existing automated tests (TestWorkflowCallInputAlignment, TestOTELHeadersSecretThreading) Replaces inline dispatch paragraph in go-code.md with cross-reference. Addresses review feedback on #4075
116be14 to
38d7a58
Compare
🔧 Fix agent — iteration 2 (human-triggered)Rebased PR branch onto latest origin/main as requested. No conflicts. The commit was cleanly replayed. All 3 changed files (AGENTS.md, docs/contributing/go-code.md, docs/contributing/workflow-contracts.md) are intact. Go tests pass (failures in internal/binary, internal/cli, internal/fetch are sandbox network restrictions, not related to this PR). Fixed (1):
Tests: passed Updated by fullsend fix agent |
|
@waynesun09 anything else here? |
|
🤖 Finished Retro · ✅ Success · Started 8:09 AM UTC · Completed 8:25 AM UTC |
Retro: PR #4075 — broaden workflow sync guidanceTimeline: Issue #4071 was filed by the retro agent on Jul 10 based on a retro of PR #3903. The code agent created PR #4075 the same day (3 files, 16 additions). The PR sat 11 days without review until a human triggered Key finding — fix agent stale-branch verification: The fix agent's first run (Jul 22, run 29900226359) incorrectly disagreed with the review agent's finding that Review quality gap: The review agent found 2 of 8 real issues. The human reviewer, using a 3-agent review squad with manual verification, found 6 additional HIGH/MEDIUM findings (incorrect vars claim, missing injection-prevention guidance, per-org chain omitting dispatch.yml hop and ADR 44 deprecation, silent-failure overgeneralization, missing anti-over-forwarding caveat, missing test-coverage reference) plus caught a new inaccuracy the fix agent introduced. Review dispatch gap: The Evidence for existing issues:
Proposals filed
|
Rename the "Dispatch workflows" section in AGENTS.md to "Workflow contracts" and expand it beyond dispatch-scaffold routing sync. The new guidance documents the general GHA constraint that reusable workflows do not inherit secrets or variables, requiring every caller to explicitly forward them. Both installation-mode chains (per-org and per-repo) are called out, along with the silent failure mode of omitted optional secrets arriving as empty strings. A review instruction is added so agents and contributors flag missing forwarding hops.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #4071
Post-script verification
agent/4071-broaden-workflow-sync-guidance)9feeb1e65455aff352d5b66a53e4857201239f5d..HEAD)