Skip to content

Addressing PR comments#1

Merged
don-petry merged 4 commits intomainfrom
chore/org-wide-agents-md
Mar 28, 2026
Merged

Addressing PR comments#1
don-petry merged 4 commits intomainfrom
chore/org-wide-agents-md

Conversation

@don-petry
Copy link
Copy Markdown
Contributor

@don-petry don-petry commented Mar 28, 2026

Summary by CodeRabbit

  • Documentation
    • Added standardized engineering practices covering mandatory test-first development, strict unit/integration test expectations, and required CI quality gates (lint/format/type checks, security scans, and coverage enforcement).
    • Established rules for handling secrets, spec-driven planning artifacts before implementation, and formal PR review resolution guidance.

Extracts common patterns from google-app-scripts, broodly, and TalkTerm
into a shared AGENTS.md that individual repos can import. Covers TDD,
pre-commit checks, CI gates, PR reviews, security, and agent guidance.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 28, 2026 13:51
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 28, 2026

📝 Walkthrough

Walkthrough

Adds AGENTS.md, an organization-wide doc prescribing mandatory TDD and coverage rules, deterministic unit/integration test expectations, pre-commit checks and CI quality gates, PR review thread resolution via GitHub GraphQL, BMAD planning-artifact requirements, and security/secrets prohibitions. (49 words)

Changes

Cohort / File(s) Summary
Organization Standards Documentation
AGENTS.md
New repository-root documentation introducing mandatory engineering standards: tests-before-code TDD, no .skip()/coverage-ignore, deterministic fast unit tests with isolated/network-free constraints and marked integration tests, pre-commit checks (format, lint, optional typecheck, full tests/coverage), CI gates (CodeQL, SonarCloud, CodeRabbit, Copilot, lint/format/type checks, unit/integration tests, enforced coverage), required PR review-thread resolution via resolveReviewThread GraphQL mutation, prohibition on committing secrets or .env files, and required BMAD planning artifacts under _bmad-output/planning-artifacts/.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Addressing PR comments' is vague and does not clearly describe the main change—introducing organization-wide standardized engineering practices in AGENTS.md. Use a more descriptive title that reflects the primary change, such as 'Add AGENTS.md with organization-wide engineering standards' or 'Introduce standardized engineering practices for petry-projects'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/org-wide-agents-md

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an organization-wide AGENTS.md intended to be imported by individual repositories, consolidating shared engineering and agent-operation standards across the petry-projects org.

Changes:

  • Introduces org-level standards for TDD, pre-commit checks, CI quality gates, and PR review handling
  • Adds security/secret-handling guidance and planning artifact conventions
  • Documents agent-operation guidelines and reference links

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread AGENTS.md Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Around line 56-60: The fenced code block containing the GraphQL example (the
line starting with "gh api graphql -f query='mutation { resolveReviewThread...")
violates markdownlint MD031; fix it by adding a blank line immediately before
the opening triple-backticks and a blank line immediately after the closing
triple-backticks so the GraphQL command block is separated from surrounding text
and the following sentence about retrieving thread IDs with a reviewThreads
query.
- Line 16: Update the policy sentence at Line 16 to distinguish
developer/local/lab runs from protected CI: change "Integration tests are
allowed but MUST be clearly marked and skippable in CI." to language that
permits tests to be skippable during local or iterative CI lanes but requires
integration test coverage in protected workflows (e.g., required pre-merge
check, nightly job, or separate required lane) as referenced by Lines 45–49;
ensure the new wording mandates clear marking and a mechanism to skip in
non-protective runs while explicitly requiring execution in the protected CI
gates.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9ef5e8ce-2d4b-4d25-a587-ad299c6b6a3e

📥 Commits

Reviewing files that changed from the base of the PR and between 37b4012 and 95b3feb.

📒 Files selected for processing (1)
  • AGENTS.md

Comment thread AGENTS.md Outdated
Comment thread AGENTS.md
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Claude Claude AI changed the title Add org-wide AGENTS.md with shared development standards Addressing PR comments Mar 28, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
AGENTS.md (1)

56-60: ⚠️ Potential issue | 🟡 Minor

Fix fenced-code spacing to satisfy markdownlint (MD031).

Add blank lines before and after the fenced block around the GraphQL command example. This issue was previously flagged and remains unresolved.

Markdownlint-compliant patch
 - Use the GitHub GraphQL API via `gh api graphql` with the `resolveReviewThread` mutation:
+
   ```bash
   gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "PRRT_..."}) { thread { id isResolved } } }'
  • Retrieve thread IDs first with a reviewThreads query on the pull request.

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @AGENTS.md around lines 56 - 60, The fenced code block containing the GraphQL
example (the triple-backtick block around the gh api graphql -f query='mutation
{ resolveReviewThread(... ) }' example) needs blank lines before and after the
opening and closing to satisfy markdownlint MD031; edit AGENTS.md to insert an empty line immediately above the opening and an empty line immediately
below the closing ```, ensuring the surrounding sentence ("Use the GitHub
GraphQL API..." and "Retrieve thread IDs first...") are separated from the
fenced block.


</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @AGENTS.md:

  • Line 28: Clarify that the “4. Tests — run the full test suite with
    coverage” instruction is a pre-commit requirement by appending a short
    parenthetical (e.g., “(required before committing/pushing)”), and update the
    guidance that currently reads about running “specific tests rather than the full
    suite when iterating” to explicitly call out that this is an iterative/dev-loop
    recommendation (e.g., “(recommended during local iteration to save time; run
    full suite before commit)”). Locate and edit the two exact strings "4. Tests
    — run the full test suite with coverage" and "specific tests rather than the
    full suite when iterating" to add the clarifying parentheticals so readers can
    easily distinguish pre-commit vs iterative expectations.

Duplicate comments:
In @AGENTS.md:

  • Around line 56-60: The fenced code block containing the GraphQL example (the
    triple-backtick block around the gh api graphql -f query='mutation {
    resolveReviewThread(... ) }' example) needs blank lines before and after the
    opening and closing to satisfy markdownlint MD031; edit AGENTS.md to insert an empty line immediately above the opening and an empty line immediately
    below the closing ```, ensuring the surrounding sentence ("Use the GitHub
    GraphQL API..." and "Retrieve thread IDs first...") are separated from the
    fenced block.

</details>

<details>
<summary>🪄 Autofix (Beta)</summary>

Fix all unresolved CodeRabbit comments on this PR:

- [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended)
- [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: Organization UI

**Review profile**: ASSERTIVE

**Plan**: Pro

**Run ID**: `5b74a978-6e67-4e00-b393-25d399871fae`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 95b3feb9d985f81c1f14018da7e73baaadfa690d and f280f532313c484a65ffefad3d53ad277ed8ccd9.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

* `AGENTS.md`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Comment thread AGENTS.md Outdated
- Replace brief planning artifacts section with prescriptive BMAD Method
  section: mandatory installation, required SDD workflow, planning
  artifact requirements, agent usage, and readiness validation
- Add comprehensive CI Quality Gates section listing all required checks:
  CodeQL, SonarCloud, CodeRabbit, GitHub Copilot, linting, formatting,
  type checking, tests, and code coverage
- Remove old generic CI section (now incorporated into new section)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (3)
AGENTS.md (3)

46-50: ⚠️ Potential issue | 🟡 Minor

Fix markdownlint MD031 around the GraphQL fenced block.

Line 47 and Line 49 still violate fenced-block spacing in this list section. Add a blank line before and after the fenced block.

Patch
 - Use the GitHub GraphQL API via `gh api graphql` with the `resolveReviewThread` mutation:
+
   ```bash
   gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "PRRT_..."}) { thread { id isResolved } } }'
  • Retrieve thread IDs first with a reviewThreads query on the pull request.
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @AGENTS.md around lines 46 - 50, The markdown fenced code block starting with

before the ```bash fence and another blank line immediately after the closing
``` fence so the code block is separated from the surrounding list item text
(ensure the `gh api graphql` example block is surrounded by one empty line above
and below).

16-16: ⚠️ Potential issue | 🟠 Major

Tighten integration-test CI policy to require a protected gate.

Line 16 says CI must always run integration tests, but the “scheduled workflow” example can bypass pre-merge protection. Require execution in at least one protected workflow.

Patch
-- Integration tests are allowed but MUST be clearly marked. They may be skipped locally during rapid iteration, but CI MUST always run them (for example, in a separate job or scheduled workflow).
+- Integration tests are allowed but MUST be clearly marked. They may be skipped locally during rapid iteration, but MUST run in at least one protected CI workflow (for example, a required pre-merge job; optionally also in a scheduled workflow).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` at line 16, Update the integration-test policy sentence (the line
currently stating "CI MUST always run them (for example, in a separate job or
scheduled workflow)") to require execution in at least one protected workflow;
change it to say CI MUST always run integration tests and at least one must be
enforced by a protected/required workflow (e.g., a required pre-merge job or
branch protection check), while allowing additional scheduled workflows for
periodic runs or local skips. Ensure the wording replaces the current
parenthetical example and explicitly mentions "protected/required workflow" or
"required pre-merge job" so the policy cannot be satisfied by only a scheduled
workflow.

28-28: ⚠️ Potential issue | 🟡 Minor

Clarify pre-commit full-suite requirement vs iterative test guidance.

Line 28 and Line 140 still read as potentially conflicting. Add explicit parentheticals to distinguish required pre-commit behavior from local iteration advice.

Patch
-4. **Tests** — run the full test suite with coverage
+4. **Tests** — run the full test suite with coverage (required before committing/pushing)

-- Prefer small, focused commands — run specific tests rather than the full suite when iterating.
+- Prefer small, focused commands — run specific tests rather than the full suite when iterating (recommended during local iteration; run full suite before commit).

Also applies to: 140-140

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` at line 28, Update the two ambiguous test instructions that
currently read like "4. **Tests** — run the full test suite with coverage" (line
showing the numbered Tests item) and the corresponding line at 140 to explicitly
distinguish required pre-commit behavior from local iterative guidance by adding
parentheticals: change the pre-commit/required instance to include "(required
before committing/pushing; full test suite with coverage)" and change the local
iteration instance to include "(optional during local development; run targeted
tests for faster feedback)"; ensure these exact phrases are applied to the two
occurrences so readers clearly understand which is mandatory and which is
advisory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@AGENTS.md`:
- Around line 46-50: The markdown fenced code block starting with ```bash in
AGENTS.md violates MD031; fix it by adding a blank line immediately before the
```bash fence and another blank line immediately after the closing ``` fence so
the code block is separated from the surrounding list item text (ensure the `gh
api graphql` example block is surrounded by one empty line above and below).
- Line 16: Update the integration-test policy sentence (the line currently
stating "CI MUST always run them (for example, in a separate job or scheduled
workflow)") to require execution in at least one protected workflow; change it
to say CI MUST always run integration tests and at least one must be enforced by
a protected/required workflow (e.g., a required pre-merge job or branch
protection check), while allowing additional scheduled workflows for periodic
runs or local skips. Ensure the wording replaces the current parenthetical
example and explicitly mentions "protected/required workflow" or "required
pre-merge job" so the policy cannot be satisfied by only a scheduled workflow.
- Line 28: Update the two ambiguous test instructions that currently read like
"4. **Tests** — run the full test suite with coverage" (line showing the
numbered Tests item) and the corresponding line at 140 to explicitly distinguish
required pre-commit behavior from local iterative guidance by adding
parentheticals: change the pre-commit/required instance to include "(required
before committing/pushing; full test suite with coverage)" and change the local
iteration instance to include "(optional during local development; run targeted
tests for faster feedback)"; ensure these exact phrases are applied to the two
occurrences so readers clearly understand which is mandatory and which is
advisory.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b3752871-11a6-4e10-80ef-c0abfa470040

📥 Commits

Reviewing files that changed from the base of the PR and between f280f53 and d76e143.

📒 Files selected for processing (1)
  • AGENTS.md

- Add blank lines around fenced code block in PR Reviews section (MD031)
- Clarify pre-commit vs iteration test requirements with cross-references
  between Pre-Commit Quality Checks and Agent Operation Guidance

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@AGENTS.md`:
- Line 131: Replace the casual phrase "fix the issue" in the sentence "If any
check fails in CI, investigate locally, fix the issue, and re-run" with a more
formal verb such as "resolve," "address," or "correct" (e.g., "resolve the
issue") to improve tone while keeping meaning and flow; update the sentence
accordingly where that exact phrase appears.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2e667a52-aa9e-4908-8f59-4fce5e5da61a

📥 Commits

Reviewing files that changed from the base of the PR and between d76e143 and e370fe7.

📒 Files selected for processing (1)
  • AGENTS.md

Comment thread AGENTS.md
- All checks above are configured as **required status checks** on the default branch.
- **Iterate until all CI checks pass.** Before requesting review or marking a PR as ready:
- Run all checks locally (format, lint, typecheck, tests, coverage)
- If any check fails in CI, investigate locally, fix the issue, and re-run
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Optional: Consider more formal verb (purely stylistic).

LanguageTool suggests replacing "fix the issue" with a more formal verb like "resolve," "address," or "correct." However, "fix" is widely accepted and clear in technical documentation, so this change is entirely optional.

♻️ Optional stylistic refinement
-  - If any check fails in CI, investigate locally, fix the issue, and re-run
+  - If any check fails in CI, investigate locally, resolve the issue, and re-run
📝 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.

Suggested change
- If any check fails in CI, investigate locally, fix the issue, and re-run
- If any check fails in CI, investigate locally, resolve the issue, and re-run
🧰 Tools
🪛 LanguageTool

[style] ~131-~131: Consider using a different verb for a more formal wording.
Context: ...check fails in CI, investigate locally, fix the issue, and re-run - Continue iter...

(FIX_RESOLVE)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` at line 131, Replace the casual phrase "fix the issue" in the
sentence "If any check fails in CI, investigate locally, fix the issue, and
re-run" with a more formal verb such as "resolve," "address," or "correct"
(e.g., "resolve the issue") to improve tone while keeping meaning and flow;
update the sentence accordingly where that exact phrase appears.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants