Repository navigation
feat: implement issue #1729 — [Phase 3] Collapse the first repo's Class-1 event-driven stubs into agent-ingress.yml and rename the required checks - #1977
Conversation
…ss-1 event-driven stubs into agent-ingress.yml and rename the required checks
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1977 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required check |
|
No description provided. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-29T20:34:43Z. |
This comment has been minimized.
This comment has been minimized.
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the agent-ingress collapse package for the pilot repository petry-projects/markets and implements a template drift guard exception for reference rows like agent-ingress.yml that are enrolled for coverage but not yet seeded. The review feedback recommends safely guarding nested property accesses in the pr-review job's concurrency group expression to prevent runtime errors, and suggests asserting specific non-zero exit codes in the BATS tests to avoid false positives on unexpected failures.
Superseded by automated re-review at
|
|
Fixed: the comment in scripts/template_stub_drift.sh (reference-rows section) named a nonexistent |
|
Acknowledged — this is a Codex usage-limit notice, not a finding on the diff. No code action is required; the PR's substantive review is being handled through the review threads and the other tooling. Noting for the record only. |
|
Acknowledged — this is a Qodo billing/trial-ended notice, not an actionable review finding on this PR. No code change is warranted; recording the acknowledgement only. |
|
Acknowledged — this is a CodeRabbit review-limit/rate-limit notice (no findings were produced this run). No code action is required; the substantive review is covered by the resolved review threads. Recording the acknowledgement only. |
|
Acknowledged — SonarCloud reports the Quality Gate passed for this PR. This is a status notice with no action required. |
Superseded by automated re-review at
|
Dev-Lead — fix-reviews (applied)Changes committed and pushed. Requested items addressed:
|
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-29T20:48:31Z. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
Acknowledged — SonarCloud reports the Quality Gate passed for this PR (0 new issues, 0 security hotspots, 0 duplication on new code). This is a status notice with no action required; recording the acknowledgement only. |
Dev-Lead — fix-reviews (no-changes)Agent reasoning |
Automated review — human attention neededThis PR has been through 3 automated review cycles since the last approval or escalation (cap: 3) without converging. Further automated review has been paused to avoid infinite loops. Please take a look manually, or close this PR if it's no longer needed. To re-engage the automated cascade with a fresh cycle budget, a human should remove the Posted by the donpetry-bot PR-review cascade. |
|
dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
Superseded by automated re-review at
|
|
No description provided. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: cde6fcef2c19208397452c289012203f9c1c787a
Review mode: triage-approved (single reviewer)
Summary
The only blocking finding from the prior review is fixed: the PR body now says Refs #1729 instead of Closes #1729, and closingIssuesReferences is empty, so merging will no longer auto-close the story before the markets companion issue is filed. The head is still cde6fce, so the code is the same as last time. That code was already found sound: a small change to the drift-guard shell logic with bats coverage, an AGENTS.md section, and an initiative doc. CI is green and every review thread is resolved.
Linked issue analysis
Issue #1729 asks for a reviewable collapse package plus a companion issue filed in petry-projects/markets. This PR delivers the package: docs/initiatives/agent-ingress-collapse-markets.md (the exact agent-ingress.yml, the eligible set with carve-outs, the old→new required-check name mapping, the baseline, the gate and rollback), the AC12 reference-row carve-out in scripts/template_stub_drift.sh, bats tests, and AGENTS.md docs. The PR now only references #1729 (Refs), so the story stays open until the companion issue is filed. This is the correct scoping.
Findings
Prior blocking finding:
Closes #1729would have auto-closed a partly done story. RESOLVED. The PR body now saysRefs #1729, and GitHub lists no closing issue references.
Changes since the prior review: none. The head SHA is the same (cde6fce); only PR metadata changed.
Non-blocking notes:
- Wording nit, carried forward: the header comment in
tests/template_stub_drift.batsstill says--emit-workflow-only REFERENCE_MANIFEST, but the matching script comment was corrected to--emit-workflow. This is cosmetic. template_drift_is_referenceis a pure membership check. Only the MISSING branch changes, from::warning::to::notice::, and only for.github/workflows/agent-ingress.yml. DRIFTED handling is unchanged, so the guard still fails on real drift. Bats tests cover both the reference row and a non-reference row.- The workflow YAML appears only inside a markdown doc, so no executing workflow is added or changed. No auth or credential handling is touched.
- Secret scan: the
run_secret_scanningMCP tool isn't available in this session. The gitleaks CI check passed, and the diff adds no credentials. - Both gemini-code-assist review threads are resolved. There are no unanswered human-reviewer questions.
CI status
Every check passed or was skipped as expected: shellcheck/ShellCheck, bats, unit-tests, Lint, actionlint, CodeQL (actions and python), SonarCloud (Quality Gate passed), gitleaks, AgentShield and Agent Security Scan, template-drift, caller-stub-freeze, check-model-pins, validate-ingress-if, and the validate-* and verify-* suites. Skipped: dependency-audit ecosystem jobs that don't apply here, dev-lead ci-relay/resume, and dependabot-automerge.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
|
pr-review approved on PARTIAL advisory evidence: 4/7 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596). |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: cde6fcef2c19208397452c289012203f9c1c787a
Review mode: triage-approved (single reviewer)
Summary
This is a confirmation review of head cde6fce (merges from main only since the last substantive commit). The PR adds a "reference row" concept to the template drift guard. A MISSING .github/workflows/agent-ingress.yml now prints a non-actionable ::notice:: instead of the generic "re-seed via seed-repo-template.sh" warning. That warning could never clear, because seed-repo-template.sh deliberately never seeds this file before the ADR-0007 pilot. The PR also documents this in AGENTS.md and adds the markets collapse-package design doc. I agree with the triage verdict. The shell change is small and pure, MISSING stays non-fatal, and only DRIFTED fails the job, as before. Both paths have bats tests: the reference row gets the notice, and non-reference rows still get the re-seed remedy.
Linked issue analysis
The PR uses Refs #1729, and closingIssuesReferences is empty, so merging won't auto-close the story. That is correct here. #1729's main acceptance criteria (the live collapse in the target repo, the branch-protection check rename, and the billed-run comparison) have to happen in the target repo and need an admin step. This PR delivers the in-repo parts: the collapse-package design doc for petry-projects/markets and the drift-guard fix (AC #12) that stops the guard from telling maintainers to hand-seed a live ingress early. Those parts address the story's in-repo scope.
Findings
No blocking findings.
- Shell logic (
scripts/template_stub_drift.sh):template_drift_is_referenceis a pure exact-match lookup over a readonly array, and it only changes the message on the MISSING branch. The exit status is unchanged: MISSING was already non-fatal.agent-ingress.ymlis enrolled inTEMPLATE_DRIFT_FILESand in seed-repo-template.sh's reference manifest, so the new branch is reachable. - Tests: there are 4 new bats cases covering both outcomes of the predicate and both annotation paths, including a regression check that non-reference MISSING rows still show the re-seed remedy. CI
batsis green. - Design doc: the markets doc is markdown only, and its YAML snippets don't run. They forward secrets only to pinned first-party
petry-projects/*reusables. The doc itself flags its open items, the §4 job-levelconcurrency:ruling and the §5 defective pins, as needing a decision before the real collapse. - Review threads: both gemini threads are resolved, the CodeAnt
--emit-workflow-onlycomment nit was fixed in the .sh, and the prior review'sCloses→Refsmetadata finding is fixed. - Nit (non-blocking): the bats header comment still says "
--emit-workflow-onlyREFERENCE_MANIFEST", the same wording CodeAnt flagged in the .sh. It's cosmetic, so it can be fixed in a follow-up. - Secret scan (MCP): the
run_secret_scanningtool isn't available in this run, so it was skipped. CI gitleaks passed.
CI status
All checks that ran passed (Lint, shellcheck/ShellCheck, bats, unit-tests, actionlint, CodeQL, SonarCloud Quality Gate, gitleaks, template-drift, caller-stub-freeze, validate-*, AgentShield). The rest were skipped because they don't apply, such as the dependency-audit ecosystems and dependabot jobs. None failed or are pending.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
…sha can never verify — comment is never minimized, dev-lead refuses to re-answer, PR blocks permanently (#1977)
…sha can never verify — comment is never minimized, dev-lead refuses to re-answer, PR blocks permanently (#1977)
…sha can never verify — comment is never minimized, dev-lead refuses to re-answer, PR blocks permanently (#1977) (#2051) * feat: implement issue #2004 — A `fixed` disposition citing the WRONG sha can never verify — comment is never minimized, dev-lead refuses to re-answer, PR blocks permanently (#1977) * fix(reviews): avoid command substitution in conditionals; align fixed-verification docs with date rule Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(reviews): whole-token matching, uncap fixing-commit search, clarify failed-pass doc Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(bot): clarify date guidance for fixed-verification — must be at or after the finding, not strictly later * docs: add missing docstrings for bot-comment and fix-reviews functions Added docstring descriptions for three functions in scripts/dev-lead-fix-reviews.sh that were missing them: - build_and_run: load a prompt template, substitute variables, and run the writer engine - resolve_dispositioned_comments: minimize PR issue comments that carry a verified disposition - handle_rate_limit: post a rate-limit marker on the PR and exit with status 2 This addresses the docstring coverage check which required 80% coverage (was at 75%). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix(reviews): re-read comment body before and after posting a corrected fixed disposition * fix(reviews): parse live comment body with jq; teach #2004 gh stub the body query Test-Change-Justification: the shared gh stub in _setup_2004 gained a case for the new live-body query (#2051 review thread PRRT_kwDOR9SdIs6otPR1); no assertion changed. * fix(reviews): unminimize a RESOLVED comment whose body changed before/after a correction Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(bot): remove out-of-scope docstrings for build_and_run and handle_rate_limit These generic utility functions are not related to disposition verification (issue #2004) and should not have docstrings added in this scope. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix(reviews): fail closed on tokenless fixed dispositions; drop generic finding tokens Tokenless findings now verify only on a commit produced by this pass (tokenless-not-this-pass otherwise); generic spans are not tokens; the fixing-commit search stops when a newer commit re-adds the token. Test-Change-Justification: path-like spans are generic per maintainer decision on PR #2051 review thread PRRT_kwDOR9SdIs6ossVo (#2004 rule 3a); the extraction test no longer expects a file path token. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(bot): strengthen token verification link and test incidental removal In scripts/lib/comment-disposition-verify.sh, add a comment documenting the limitation identified by CodeAnt: when a finding has multiple distinct issues (multiple tokens), the verification accepts a commit that removes only one token, even if the underlying issues remain. The verification could be strengthened by requiring all tokens to be removed or by checking that removals are focused on the finding, not incidental changes. In tests/dev-lead/unit/test_comment_disposition_verify.bats, add a test case for incidental token removal in an unrelated change to document the limitation and provide a test for future strengthening work. This addresses the medium-risk finding from CodeAnt about strengthening the link between the diff and the finding. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix(reviews): strip diff marker before token match; fix #2004 gh stub; inclusive date wording - _cdv_diff_side strips the one-char diff marker so a token at column 0 of a removed line is not flanked by `-` (Graphite). - fix-reviews.md: `fixed` date rule reads "at or after" (CodeRabbit). - test_fix_reviews.bats: the #2004 gh stub returned reviewThreads without pageInfo and was shadowed by the comments case, so the #2056 fail-closed thread read aborted every #2004 test (red `unit` CI job). Test-Change-Justification: #2004 stub predates the #2056 fail-closed thread read; it returned a malformed reviewThreads page and was matched first by the comments case, so the tests aborted before exercising the verdict. Only the stub changed, no assertion. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> --------- Co-authored-by: don-petry <{}+don-petry@users.noreply.github.com> Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
2 participants
Problem
[Phase 3] Collapse the first repo's Class-1 event-driven stubs into agent-ingress.yml and rename the required checks
From the issue: As a fleet workflow maintainer, I want collapse the eligible Class-1 event-driven thin-caller stubs in the first target repo into a single agent-ingress.yml and rename the branch-protection required checks to the new per-job names in the same change, so that the first repo demonstrates the ADR-0007 collapse end-to-end with zero dropped triggers and no protection gap, validating the whole toolchain.
Risk
Low — changes automation shell logic under scripts/, covered by shellcheck (--severity=warning) and the bats suite.
Test plan
Tests added/updated:
tests/template_stub_drift.bats. Verification:bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.Rollback
Revert this PR. No non-revertible side effects (no tags, migrations, or external state).
Monitoring
This PR's Lint (shellcheck) and bats checks show pass/fail; watch subsequent dev-lead / pr-review runs for behavioral regressions.
Refs #1729