From daa824bf112ec81cd0810693fc564956884fd80d Mon Sep 17 00:00:00 2001 From: Rodrigo Nardi Date: Sat, 30 May 2026 20:51:04 -0300 Subject: [PATCH 1/5] Refactor plan fetching logic to support legacy pull requests without associated plans --- lib/github/re_run/command.rb | 4 +- lib/github/re_run/comment.rb | 6 ++- spec/lib/github/re_run/command_spec.rb | 51 ++++++++++++++++++++++++++ spec/lib/github/re_run/comment_spec.rb | 48 ++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 2 deletions(-) diff --git a/lib/github/re_run/command.rb b/lib/github/re_run/command.rb index 6cdc2ab..7bf2eea 100644 --- a/lib/github/re_run/command.rb +++ b/lib/github/re_run/command.rb @@ -40,7 +40,9 @@ def start private def suite_by_plan(check_suite) - check_suite.pull_request.plans.each do |plan| + plans = check_suite.pull_request.plans.presence || Plan.where(github_repo_name: repo) + + plans.each do |plan| CreateExecutionByCommand .delay(run_at: TIMER.seconds.from_now.utc, queue: 'create_execution_by_command') .create(plan.id, check_suite.id, @payload) diff --git a/lib/github/re_run/comment.rb b/lib/github/re_run/comment.rb index 670cd64..e743aba 100644 --- a/lib/github/re_run/comment.rb +++ b/lib/github/re_run/comment.rb @@ -41,7 +41,7 @@ def confirm_and_start github_reaction_feedback(comment_id) - @pull_request.plans.each do |plan| + fetch_plans.each do |plan| CreateExecutionByComment .delay(run_at: TIMER.seconds.from_now.utc, queue: 'create_execution_by_comment') .create(@pull_request.id, @payload, plan) @@ -50,6 +50,10 @@ def confirm_and_start [200, 'Scheduled Plan Runs'] end + def fetch_plans + @pull_request.plans.presence || Plan.where(github_repo_name: repo) + end + def github_reaction_feedback(comment_id) return if comment_id.nil? diff --git a/spec/lib/github/re_run/command_spec.rb b/spec/lib/github/re_run/command_spec.rb index a05476a..59c964a 100644 --- a/spec/lib/github/re_run/command_spec.rb +++ b/spec/lib/github/re_run/command_spec.rb @@ -91,6 +91,57 @@ end end + context 'when the pull request has no associated plans (legacy)' do + let(:plan) { create(:plan, github_repo_name: 'Unit/Test') } + let(:pull_request) { create(:pull_request, repository: 'Unit/Test', plans: []) } + let(:check_suite) { create(:check_suite, :with_running_ci_jobs, pull_request: pull_request) } + let(:fake_plan_run) { BambooCi::PlanRun.new(nil, plan) } + let(:check_suites) { CheckSuite.where(commit_sha_ref: check_suite.commit_sha_ref) } + let(:payload) do + { + 'action' => 'created', + 'check_suite' => { + 'head_sha' => check_suite.commit_sha_ref, + 'pull_requests' => [{ 'number' => pull_request.github_pr_id }] + }, + 'repository' => { 'full_name' => 'Unit/Test' } + } + end + + before do + plan + check_suite + + allow(Octokit::Client).to receive(:new).and_return(fake_client) + allow(fake_client).to receive(:find_app_installations).and_return([{ 'id' => 1 }]) + allow(fake_client).to receive(:create_app_installation_access_token).and_return({ 'token' => 1 }) + + allow(Github::Check).to receive(:new).and_return(fake_github_check) + allow(fake_github_check).to receive(:create).and_return(check_suite) + allow(fake_github_check).to receive(:add_comment) + allow(fake_github_check).to receive(:cancelled) + allow(fake_github_check).to receive(:in_progress) + allow(fake_github_check).to receive(:queued) + allow(fake_github_check).to receive(:skipped) + allow(fake_github_check).to receive(:comment_reaction_thumb_up) + allow(fake_github_check).to receive(:fetch_username).and_return({}) + allow(fake_github_check).to receive(:check_runs_for_ref).and_return({}) + + allow(BambooCi::PlanRun).to receive(:new).and_return(fake_plan_run) + allow(fake_plan_run).to receive(:start_plan).and_return(200) + allow(fake_plan_run).to receive(:bamboo_reference).and_return('UNIT-TEST-1') + + allow(BambooCi::StopPlan).to receive(:build) + allow(BambooCi::RunningPlan).to receive(:fetch).and_return([]) + end + + it 'must return success using plans from repository' do + expect(rerun.start).to eq([200, 'Scheduled Plan Runs']) + expect(check_suites.size).to eq(2) + expect(check_suites.last.re_run).to be_truthy + end + end + context 'when receives a valid command but invalid PR ID' do let(:check_suite) { create(:check_suite, :with_running_ci_jobs) } let(:ci_jobs) do diff --git a/spec/lib/github/re_run/comment_spec.rb b/spec/lib/github/re_run/comment_spec.rb index 1ebb0f3..16e042e 100644 --- a/spec/lib/github/re_run/comment_spec.rb +++ b/spec/lib/github/re_run/comment_spec.rb @@ -303,6 +303,54 @@ end end + context 'when the pull request has no associated plans (legacy)' do + let(:plan) { create(:plan, github_repo_name: 'test') } + let(:pull_request) { create(:pull_request, github_pr_id: 22, repository: 'test', plans: []) } + let(:check_suite) { create(:check_suite, :with_running_ci_jobs, pull_request: pull_request) } + let(:fake_plan_run) { BambooCi::PlanRun.new(nil, plan) } + let(:check_suites) { CheckSuite.where(commit_sha_ref: check_suite.commit_sha_ref) } + let(:payload) do + { + 'action' => 'created', + 'comment' => { 'body' => "CI:rerun ##{check_suite.commit_sha_ref}", 'id' => 1, 'user' => { 'login' => 'John' } }, + 'repository' => { 'full_name' => 'test' }, + 'issue' => { 'number' => pull_request.github_pr_id } + } + end + + before do + plan + check_suite + + allow(Octokit::Client).to receive(:new).and_return(fake_client) + allow(fake_client).to receive(:find_app_installations).and_return([{ 'id' => 1 }]) + allow(fake_client).to receive(:create_app_installation_access_token).and_return({ 'token' => 1 }) + + allow(Github::Check).to receive(:new).and_return(fake_github_check) + allow(fake_github_check).to receive(:create).and_return(check_suite) + allow(fake_github_check).to receive(:add_comment) + allow(fake_github_check).to receive(:cancelled) + allow(fake_github_check).to receive(:in_progress) + allow(fake_github_check).to receive(:queued) + allow(fake_github_check).to receive(:comment_reaction_thumb_up) + allow(fake_github_check).to receive(:fetch_username).and_return({}) + allow(fake_github_check).to receive(:check_runs_for_ref).and_return({}) + + allow(BambooCi::PlanRun).to receive(:new).and_return(fake_plan_run) + allow(fake_plan_run).to receive(:start_plan).and_return(200) + allow(fake_plan_run).to receive(:bamboo_reference).and_return('UNIT-TEST-1') + + allow(BambooCi::StopPlan).to receive(:build) + allow(BambooCi::RunningPlan).to receive(:fetch).and_return([]) + end + + it 'must return success using plans from repository' do + expect(rerun.start).to eq([200, 'Scheduled Plan Runs']) + expect(check_suites.size).to eq(2) + expect(check_suites.last.re_run).to be_truthy + end + end + context 'when receives an invalid pull request' do let(:pull_request) { create(:pull_request, github_pr_id: 12, repository: 'test') } let(:check_suite) { create(:check_suite, :with_running_ci_jobs, pull_request: pull_request) } From 0c0f81cfad603c800130aa9887570e6848e425ab Mon Sep 17 00:00:00 2001 From: Rodrigo Nardi Date: Mon, 1 Jun 2026 09:54:29 -0300 Subject: [PATCH 2/5] Add legacy single-plan migration tests and update check suite handling --- lib/models/pull_request.rb | 4 +- spec/lib/github/build_plan_spec.rb | 67 +++++++++++++++++++ spec/workers/create_execution_by_plan_spec.rb | 24 +++++++ workers/create_execution_by_plan.rb | 4 +- 4 files changed, 96 insertions(+), 3 deletions(-) diff --git a/lib/models/pull_request.rb b/lib/models/pull_request.rb index 48edcf1..99bafd4 100644 --- a/lib/models/pull_request.rb +++ b/lib/models/pull_request.rb @@ -22,7 +22,9 @@ class PullRequest < ActiveRecord::Base def finished? return true if check_suites.nil? or check_suites.empty? - current_execution_by_plan(plan).finished? + return plans.all? { |p| current_execution_by_plan(p)&.finished? } if plans.any? + + check_suites.last&.finished? || true end def current_execution?(check_suite) diff --git a/spec/lib/github/build_plan_spec.rb b/spec/lib/github/build_plan_spec.rb index de9b2b6..d6018b6 100644 --- a/spec/lib/github/build_plan_spec.rb +++ b/spec/lib/github/build_plan_spec.rb @@ -135,6 +135,73 @@ end end + describe 'Legacy single-plan migration' do + let!(:plan) { create(:plan, github_repo_name: repo) } + let(:pr_number) { rand(1_000_000) } + let(:repo) { 'UnitTest/legacy' } + let(:author) { 'Legacy User' } + let(:action) { 'synchronize' } + let(:fake_translation) { create(:stage_configuration) } + let(:ci_jobs) do + [{ name: 'Build', job_ref: 'BUILD-1', stage: fake_translation.bamboo_stage_name }] + end + + let(:pull_request) { create(:pull_request, github_pr_id: pr_number, repository: repo, author: author, plans: []) } + let!(:legacy_check_suite) { create(:check_suite, :with_running_ci_jobs, pull_request: pull_request) } + + let(:payload) do + { + 'action' => action, + 'number' => pr_number, + 'pull_request' => { + 'user' => { 'login' => author }, + 'head' => { 'ref' => 'legacy-branch', 'sha' => Digest::SHA2.hexdigest('legacy') }, + 'base' => { 'ref' => 'master', 'sha' => Digest::SHA2.hexdigest('base') } + }, + 'repository' => { 'full_name' => repo } + } + end + + before do + allow(Octokit::Client).to receive(:new).and_return(fake_client) + allow(fake_client).to receive(:find_app_installations).and_return([{ 'id' => 1 }]) + allow(fake_client).to receive(:create_app_installation_access_token).and_return({ 'token' => 1 }) + + allow(BambooCi::PlanRun).to receive(:new).and_return(fake_plan_run) + allow(fake_plan_run).to receive(:start_plan).and_return(200) + allow(fake_plan_run).to receive(:bamboo_reference).and_return('BUILD-1') + + allow(Github::Check).to receive(:new).and_return(fake_github_check) + allow(fake_github_check).to receive(:create).and_return(fake_check_run) + allow(fake_github_check).to receive(:in_progress).and_return(fake_check_run) + allow(fake_github_check).to receive(:queued).and_return(fake_check_run) + allow(fake_github_check).to receive(:cancelled) + allow(fake_github_check).to receive(:fetch_username).and_return({}) + allow(fake_github_check).to receive(:check_runs_for_ref).and_return({}) + + allow(BambooCi::RunningPlan).to receive(:fetch).and_return(ci_jobs) + allow(BambooCi::StopPlan).to receive(:build) + allow(BambooCi::StopPlan).to receive(:comment) + end + + it 'migrates the PR to the multiple-plan model and schedules a new execution' do + expect(build_plan.create).to eq([200, 'Scheduled Plan Runs']) + expect(pull_request.reload.plans).to include(plan) + end + + it 'stops the in-progress legacy check suite' do + build_plan.create + expect(legacy_check_suite.ci_jobs.reload.map(&:status)).to all(eq('cancelled')) + end + + it 'creates a new check suite associated with the plan' do + build_plan.create + new_suite = pull_request.check_suites.reload.last + expect(new_suite).not_to eq(legacy_check_suite) + expect(new_suite.plan).to eq(plan) + end + end + describe 'Invalid commands' do let!(:plan) { create(:plan, github_repo_name: repo) } diff --git a/spec/workers/create_execution_by_plan_spec.rb b/spec/workers/create_execution_by_plan_spec.rb index 144e40f..d6bf782 100644 --- a/spec/workers/create_execution_by_plan_spec.rb +++ b/spec/workers/create_execution_by_plan_spec.rb @@ -79,5 +79,29 @@ expect(described_class.create(pull_request.id, payload, 999)).to eq([422, 'Plan not found']) end end + + context 'when the pull request has a legacy check suite with no plan_id' do + let(:legacy_pr) { create(:pull_request, plans: []) } + let(:plan) { create(:plan) } + let!(:legacy_check_suite) { create(:check_suite, :with_running_ci_jobs, pull_request: legacy_pr) } + + before do + allow(BambooCi::StopPlan).to receive(:build) + allow(BambooCi::StopPlan).to receive(:comment) + allow(fake_github_check).to receive(:cancelled) + end + + it 'finds the legacy check suite and stops it' do + described_class.create(legacy_pr.id, payload, plan.id) + expect(legacy_check_suite.ci_jobs.reload.map(&:status)).to all(eq('cancelled')) + end + + it 'creates a new check suite associated with the plan' do + described_class.create(legacy_pr.id, payload, plan.id) + new_suite = legacy_pr.check_suites.reload.last + expect(new_suite).not_to eq(legacy_check_suite) + expect(new_suite.plan).to eq(plan) + end + end end end diff --git a/workers/create_execution_by_plan.rb b/workers/create_execution_by_plan.rb index 09f7a1d..2ac46df 100644 --- a/workers/create_execution_by_plan.rb +++ b/workers/create_execution_by_plan.rb @@ -142,8 +142,8 @@ def mark_as_cancelled_stages def fetch_last_check_suite(plan) @last_check_suite = CheckSuite - .joins(pull_request: :plans) - .where(pull_request: { id: @pull_request.id, plans: { name: plan.name } }) + .where(pull_request: @pull_request) + .where(plan: [plan, nil]) .last end From aec72c1e6556177109bb8b47567bea104f412a4a Mon Sep 17 00:00:00 2001 From: Rodrigo Nardi Date: Mon, 1 Jun 2026 10:12:03 -0300 Subject: [PATCH 3/5] Add AI Agent Collaboration Guide to enhance coding consistency --- AGENTS.md | 360 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 360 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..7d5afb9 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,360 @@ +# AGENTS.md — AI Agent Collaboration Guide + +This file provides context and conventions for AI coding agents (Claude Code, Copilot, Cursor, etc.) contributing to this project. Follow these patterns to produce code that is consistent with the existing codebase. + +--- + +## Project Overview + +**netdef-ci-github-app** is a Ruby/Sinatra webhook server that bridges GitHub pull requests and a Bamboo CI system. It receives GitHub events, triggers CI builds, polls for results, updates GitHub Check Runs, and sends Slack notifications. + +**Stack:** +- Ruby 3.1+ / Sinatra / Rack / Puma +- PostgreSQL via `otr-activerecord` (ActiveRecord without Rails) +- Delayed Job (background workers — not Sidekiq) +- Octokit (GitHub API) +- RSpec + FactoryBot + WebMock (tests) +- Rubocop 1.56 (linting — enforced in CI) + +--- + +## Repository Layout + +``` +app/github_app.rb # Sinatra entry point — routes only, no business logic +lib/ + bamboo_ci/ # All Bamboo REST API interactions + github/ # GitHub webhook handlers and Check API wrappers + build/ # Submodules for build orchestration + retry/ # Comment-triggered retry logic + re_run/ # Check suite re-run logic + plan_execution/ # Handlers for execution completion + topotest_failures/ # Failure log parsers + parsers/ # PR commit parsers + models/ # ActiveRecord models (12 models) + helpers/ # Cross-cutting concerns: config, logging, metrics, auth + slack/ # Slack API client + slack_bot/ # Notification formatters +workers/ # Delayed Job worker classes +spec/ + app/ # Integration tests (Rack::Test) + lib/ # Unit tests — mirror lib/ structure + workers/ # Worker unit tests + factories/ # FactoryBot factories + support/ # RSpec helpers (factory_bot.rb, webmock.rb) +config/ + setup.rb # Boot sequence — load order matters here + delayed_job.rb # Worker tuning (max_attempts, sleep_delay, max_run_time) + database.yml # PostgreSQL connection by environment +db/migrate/ # Numbered migrations only — never edit schema.rb directly +``` + +--- + +## Code Conventions + +### File Header + +Every Ruby file starts with this exact header block (replace `filename.rb` and the year accordingly): + +```ruby +# SPDX-License-Identifier: BSD-2-Clause +# +# filename.rb +# Part of NetDEF CI System +# +# Copyright (c) by +# Network Device Education Foundation, Inc. ("NetDEF") +# +# frozen_string_literal: true +``` + +Include this header on all new files. The order matters: license identifier first, then filename + project line, then copyright, then frozen string literal. + +### Naming + +| Construct | Convention | Example | +|-----------|-----------|---------| +| Classes / Modules | PascalCase | `BambooCi::PlanRun` | +| Methods | snake_case | `fetch_executions` | +| Constants | UPPER_SNAKE_CASE | `MAX_RETRY_ATTEMPTS` | +| DB tables | plural snake_case | `ci_jobs`, `check_suites` | +| AR models | singular PascalCase | `CiJob`, `CheckSuite` | +| Files | snake_case matching class | `plan_run.rb` for `PlanRun` | + +### Modules & Namespacing + +Group classes by domain, not by layer: + +``` +lib/bamboo_ci/api.rb → module BambooCi, class Api +lib/github/check.rb → module Github, class Check +lib/slack_bot/stage.rb → module SlackBot, class Stage +``` + +Do not put business logic in `app/github_app.rb`. Routes delegate immediately: + +```ruby +# Good +post '/*' do + Github::BuildPlan.new(payload).create +end + +# Bad — inline logic in the route +post '/*' do + pull_request = PullRequest.find_or_create_by(...) + # ... 50 lines of logic +end +``` + +### Method Length + +Rubocop enforces **20 lines max**. Extract private helpers rather than writing long methods. + +### Class / Module Length + +Rubocop enforces **200 lines max**. Split responsibilities into submodules (see `lib/github/build/`, `lib/github/retry/`). + +### No Documentation Cops + +`Style/Documentation` is disabled. Do not add class-level docblock comments unless there is a non-obvious invariant to explain. One-line comments for _why_, not _what_. + +--- + +## Models + +### Enums + +Status enums follow this shared pattern across `CiJob`, `Stage`, and `AuditStatus`: + +```ruby +enum status: { + queued: 0, + in_progress: 1, + success: 2, + cancelled: -1, + failure: -2, + skipped: -3 +} +``` + +Use the symbol form everywhere: `ci_job.success!`, `stage.in_progress?`, `CiJob.where(status: :failure)`. + +### Associations + +Declare associations at the top of the model, before validations and scopes: + +```ruby +class CiJob < ApplicationRecord + belongs_to :check_suite + belongs_to :stage + has_many :topotest_failures, dependent: :destroy + has_many :audit_statuses, as: :auditable, dependent: :destroy + + enum status: { ... } + + scope :failure_only, -> { where(status: :failure) } +end +``` + +### Migrations + +- Always create a numbered migration: `rails generate migration AddColumnToTable` or write by hand following `db/migrate/` naming. +- Never edit `db/schema.rb` by hand — it is regenerated by `rake db:migrate:reset`. +- Validate that `schema.rb` matches migrations in CI: `diff <(rake db:schema:dump) db/schema.rb`. + +--- + +## Workers (Delayed Job) + +Workers are plain Ruby classes enqueued with `.delay`: + +```ruby +# Enqueue +CreateExecutionByPlan.new.delay(queue: 2).perform(check_suite_id: suite.id) + +# Worker +class CreateExecutionByPlan + def perform(check_suite_id:) + suite = CheckSuite.find(check_suite_id) + # ... + end +end +``` + +**Rules:** +- Workers live in `workers/`, not in `lib/`. +- Workers do not inherit from `ActiveJob::Base` — use plain Ruby classes. +- Keep workers thin: delegate logic to `lib/` classes. +- Queue numbers 0–9 are meaningful (see `config.ru` for priority tiers). +- Max 5 attempts, 5-minute timeout per job (configured in `config/delayed_job.rb`). + +--- + +## Testing + +### Framework + +RSpec with `--format=documentation --order=random`. Always run with `bundle exec rspec`. + +### Test File Location + +Mirror the source path: + +``` +lib/github/build_plan.rb → spec/lib/github/build_plan_spec.rb +workers/ci_job_status.rb → spec/workers/ci_job_status_spec.rb +app/github_app.rb → spec/app/github_app_spec.rb +``` + +### Structure Template + +```ruby +# frozen_string_literal: true + +describe Github::BuildPlan do + let(:payload) { create(:pull_request_payload) } + + describe '#create' do + context 'when the repository is configured' do + before { allow_any_instance_of(Github::Check).to receive(:create).and_return(true) } + + it 'creates a check suite' do + expect { described_class.new(payload).create }.to change(CheckSuite, :count).by(1) + end + end + + context 'when the repository is not configured' do + it 'returns early without creating records' do + expect { described_class.new(payload).create }.not_to change(CheckSuite, :count) + end + end + end +end +``` + +### Factories + +Factories live in `spec/factories/`. Use traits for associations, not nested factories: + +```ruby +FactoryBot.define do + factory :check_suite do + commit_sha_ref { Faker::Alphanumeric.alphanumeric(number: 40) } + author { Faker::Internet.username } + + trait :with_ci_jobs do + after(:create) do |suite| + create_list(:ci_job, 3, check_suite: suite) + end + end + end +end +``` + +### HTTP Mocking + +All external HTTP calls must be mocked with WebMock. No real network calls in tests: + +```ruby +before do + stub_request(:post, %r{bamboo/rest/api/latest/queue}) + .to_return(status: 200, body: { buildResultKey: 'PROJ-123' }.to_json) +end +``` + +### Coverage Requirements + +SimpleCov enforces **90% branch coverage minimum per group**. Use `:nocov:` only for genuinely untestable blocks (e.g., rescue blocks for infrastructure failures): + +```ruby +# :nocov: +rescue StandardError => e + logger.error(e.message) +# :nocov: +end +``` + +--- + +## Configuration + +Runtime configuration is loaded from `config.yml` (not committed — see `config_template.yml`): + +```ruby +config = GitHubApp::Configuration.instance +config.reload # re-reads YAML from disk +config.all_logins # GitHub app login names +config.bamboo_url # Bamboo base URL +``` + +Never hardcode URLs, credentials, or repo names. Always read from `GitHubApp::Configuration.instance`. + +--- + +## Logging + +Use the project logger — do not use `puts` or `p`: + +```ruby +logger = GithubLogger.instance.create('github_app.log', Logger::INFO) +logger.info { "[#{self.class}] Starting plan #{plan.id}" } +logger.error { "[#{self.class}] Failed: #{e.message}" } +``` + +Log file names map to components (see `lib/helpers/github_logger.rb`). Use the closest existing log file for your component. + +--- + +## Metrics + +Increment Prometheus counters/histograms for any new external call or user-visible operation: + +```ruby +PrometheusMet.instance.http_requests.increment(labels: { method: 'POST', status: '200' }) +``` + +Do not add new metric names without first checking `lib/helpers/prometheus_metrics.rb` for an existing one that fits. + +--- + +## CI/CD + +The GitHub Actions workflow (`.github/workflows/ruby.yml`) runs on every push/PR: + +1. **Rubocop** — lint check via reviewdog (inline PR comments on violations). +2. **RSpec** — full test suite with a real PostgreSQL 14 service. + +Before opening a PR: + +```bash +bundle exec rubocop # must pass with zero offenses +bundle exec rspec # must pass at ≥90% coverage +rake db:migrate:reset # verify migrations apply cleanly +``` + +--- + +## Common Pitfalls + +- **Do not use `ActiveJob`** — this project uses Delayed Job with plain Ruby workers. +- **Do not call external services from within a Sinatra route** — enqueue a worker instead. +- **Do not modify `db/schema.rb` by hand** — generate a migration. +- **Do not add logic to `app/github_app.rb`** — it should only instantiate and delegate. +- **Do not stub `Time.now` or `Date.today` globally** — use `Timecop` if a test requires time control (not currently a dependency — add it if needed). +- **Do not create new queue numbers** beyond 0–9 without updating `config.ru`. +- **Run `rubocop -A` before committing** — unformatted code fails CI immediately. + +--- + +## Checklist for New Features + +- [ ] New files include the frozen string literal comment and SPDX header. +- [ ] Business logic lives in `lib/`, not in `app/`, `workers/`, or models. +- [ ] New ActiveRecord model includes a migration and status enum (if applicable). +- [ ] New Delayed Job worker lives in `workers/` and delegates to `lib/`. +- [ ] External HTTP calls are wrapped in a class under `lib/bamboo_ci/` or `lib/github/`. +- [ ] Tests mirror the source path and achieve ≥90% branch coverage. +- [ ] WebMock stubs cover all new HTTP calls in tests. +- [ ] `rubocop` passes with no new offenses. +- [ ] `rake db:migrate:reset && bundle exec rspec` passes locally. \ No newline at end of file From 224b95e42024040601bba3ee36453d14682a8250 Mon Sep 17 00:00:00 2001 From: Rodrigo Nardi Date: Mon, 1 Jun 2026 10:12:03 -0300 Subject: [PATCH 4/5] Add AI Agent Collaboration Guide to enhance coding consistency --- AGENTS.md | 360 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 360 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..7d5afb9 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,360 @@ +# AGENTS.md — AI Agent Collaboration Guide + +This file provides context and conventions for AI coding agents (Claude Code, Copilot, Cursor, etc.) contributing to this project. Follow these patterns to produce code that is consistent with the existing codebase. + +--- + +## Project Overview + +**netdef-ci-github-app** is a Ruby/Sinatra webhook server that bridges GitHub pull requests and a Bamboo CI system. It receives GitHub events, triggers CI builds, polls for results, updates GitHub Check Runs, and sends Slack notifications. + +**Stack:** +- Ruby 3.1+ / Sinatra / Rack / Puma +- PostgreSQL via `otr-activerecord` (ActiveRecord without Rails) +- Delayed Job (background workers — not Sidekiq) +- Octokit (GitHub API) +- RSpec + FactoryBot + WebMock (tests) +- Rubocop 1.56 (linting — enforced in CI) + +--- + +## Repository Layout + +``` +app/github_app.rb # Sinatra entry point — routes only, no business logic +lib/ + bamboo_ci/ # All Bamboo REST API interactions + github/ # GitHub webhook handlers and Check API wrappers + build/ # Submodules for build orchestration + retry/ # Comment-triggered retry logic + re_run/ # Check suite re-run logic + plan_execution/ # Handlers for execution completion + topotest_failures/ # Failure log parsers + parsers/ # PR commit parsers + models/ # ActiveRecord models (12 models) + helpers/ # Cross-cutting concerns: config, logging, metrics, auth + slack/ # Slack API client + slack_bot/ # Notification formatters +workers/ # Delayed Job worker classes +spec/ + app/ # Integration tests (Rack::Test) + lib/ # Unit tests — mirror lib/ structure + workers/ # Worker unit tests + factories/ # FactoryBot factories + support/ # RSpec helpers (factory_bot.rb, webmock.rb) +config/ + setup.rb # Boot sequence — load order matters here + delayed_job.rb # Worker tuning (max_attempts, sleep_delay, max_run_time) + database.yml # PostgreSQL connection by environment +db/migrate/ # Numbered migrations only — never edit schema.rb directly +``` + +--- + +## Code Conventions + +### File Header + +Every Ruby file starts with this exact header block (replace `filename.rb` and the year accordingly): + +```ruby +# SPDX-License-Identifier: BSD-2-Clause +# +# filename.rb +# Part of NetDEF CI System +# +# Copyright (c) by +# Network Device Education Foundation, Inc. ("NetDEF") +# +# frozen_string_literal: true +``` + +Include this header on all new files. The order matters: license identifier first, then filename + project line, then copyright, then frozen string literal. + +### Naming + +| Construct | Convention | Example | +|-----------|-----------|---------| +| Classes / Modules | PascalCase | `BambooCi::PlanRun` | +| Methods | snake_case | `fetch_executions` | +| Constants | UPPER_SNAKE_CASE | `MAX_RETRY_ATTEMPTS` | +| DB tables | plural snake_case | `ci_jobs`, `check_suites` | +| AR models | singular PascalCase | `CiJob`, `CheckSuite` | +| Files | snake_case matching class | `plan_run.rb` for `PlanRun` | + +### Modules & Namespacing + +Group classes by domain, not by layer: + +``` +lib/bamboo_ci/api.rb → module BambooCi, class Api +lib/github/check.rb → module Github, class Check +lib/slack_bot/stage.rb → module SlackBot, class Stage +``` + +Do not put business logic in `app/github_app.rb`. Routes delegate immediately: + +```ruby +# Good +post '/*' do + Github::BuildPlan.new(payload).create +end + +# Bad — inline logic in the route +post '/*' do + pull_request = PullRequest.find_or_create_by(...) + # ... 50 lines of logic +end +``` + +### Method Length + +Rubocop enforces **20 lines max**. Extract private helpers rather than writing long methods. + +### Class / Module Length + +Rubocop enforces **200 lines max**. Split responsibilities into submodules (see `lib/github/build/`, `lib/github/retry/`). + +### No Documentation Cops + +`Style/Documentation` is disabled. Do not add class-level docblock comments unless there is a non-obvious invariant to explain. One-line comments for _why_, not _what_. + +--- + +## Models + +### Enums + +Status enums follow this shared pattern across `CiJob`, `Stage`, and `AuditStatus`: + +```ruby +enum status: { + queued: 0, + in_progress: 1, + success: 2, + cancelled: -1, + failure: -2, + skipped: -3 +} +``` + +Use the symbol form everywhere: `ci_job.success!`, `stage.in_progress?`, `CiJob.where(status: :failure)`. + +### Associations + +Declare associations at the top of the model, before validations and scopes: + +```ruby +class CiJob < ApplicationRecord + belongs_to :check_suite + belongs_to :stage + has_many :topotest_failures, dependent: :destroy + has_many :audit_statuses, as: :auditable, dependent: :destroy + + enum status: { ... } + + scope :failure_only, -> { where(status: :failure) } +end +``` + +### Migrations + +- Always create a numbered migration: `rails generate migration AddColumnToTable` or write by hand following `db/migrate/` naming. +- Never edit `db/schema.rb` by hand — it is regenerated by `rake db:migrate:reset`. +- Validate that `schema.rb` matches migrations in CI: `diff <(rake db:schema:dump) db/schema.rb`. + +--- + +## Workers (Delayed Job) + +Workers are plain Ruby classes enqueued with `.delay`: + +```ruby +# Enqueue +CreateExecutionByPlan.new.delay(queue: 2).perform(check_suite_id: suite.id) + +# Worker +class CreateExecutionByPlan + def perform(check_suite_id:) + suite = CheckSuite.find(check_suite_id) + # ... + end +end +``` + +**Rules:** +- Workers live in `workers/`, not in `lib/`. +- Workers do not inherit from `ActiveJob::Base` — use plain Ruby classes. +- Keep workers thin: delegate logic to `lib/` classes. +- Queue numbers 0–9 are meaningful (see `config.ru` for priority tiers). +- Max 5 attempts, 5-minute timeout per job (configured in `config/delayed_job.rb`). + +--- + +## Testing + +### Framework + +RSpec with `--format=documentation --order=random`. Always run with `bundle exec rspec`. + +### Test File Location + +Mirror the source path: + +``` +lib/github/build_plan.rb → spec/lib/github/build_plan_spec.rb +workers/ci_job_status.rb → spec/workers/ci_job_status_spec.rb +app/github_app.rb → spec/app/github_app_spec.rb +``` + +### Structure Template + +```ruby +# frozen_string_literal: true + +describe Github::BuildPlan do + let(:payload) { create(:pull_request_payload) } + + describe '#create' do + context 'when the repository is configured' do + before { allow_any_instance_of(Github::Check).to receive(:create).and_return(true) } + + it 'creates a check suite' do + expect { described_class.new(payload).create }.to change(CheckSuite, :count).by(1) + end + end + + context 'when the repository is not configured' do + it 'returns early without creating records' do + expect { described_class.new(payload).create }.not_to change(CheckSuite, :count) + end + end + end +end +``` + +### Factories + +Factories live in `spec/factories/`. Use traits for associations, not nested factories: + +```ruby +FactoryBot.define do + factory :check_suite do + commit_sha_ref { Faker::Alphanumeric.alphanumeric(number: 40) } + author { Faker::Internet.username } + + trait :with_ci_jobs do + after(:create) do |suite| + create_list(:ci_job, 3, check_suite: suite) + end + end + end +end +``` + +### HTTP Mocking + +All external HTTP calls must be mocked with WebMock. No real network calls in tests: + +```ruby +before do + stub_request(:post, %r{bamboo/rest/api/latest/queue}) + .to_return(status: 200, body: { buildResultKey: 'PROJ-123' }.to_json) +end +``` + +### Coverage Requirements + +SimpleCov enforces **90% branch coverage minimum per group**. Use `:nocov:` only for genuinely untestable blocks (e.g., rescue blocks for infrastructure failures): + +```ruby +# :nocov: +rescue StandardError => e + logger.error(e.message) +# :nocov: +end +``` + +--- + +## Configuration + +Runtime configuration is loaded from `config.yml` (not committed — see `config_template.yml`): + +```ruby +config = GitHubApp::Configuration.instance +config.reload # re-reads YAML from disk +config.all_logins # GitHub app login names +config.bamboo_url # Bamboo base URL +``` + +Never hardcode URLs, credentials, or repo names. Always read from `GitHubApp::Configuration.instance`. + +--- + +## Logging + +Use the project logger — do not use `puts` or `p`: + +```ruby +logger = GithubLogger.instance.create('github_app.log', Logger::INFO) +logger.info { "[#{self.class}] Starting plan #{plan.id}" } +logger.error { "[#{self.class}] Failed: #{e.message}" } +``` + +Log file names map to components (see `lib/helpers/github_logger.rb`). Use the closest existing log file for your component. + +--- + +## Metrics + +Increment Prometheus counters/histograms for any new external call or user-visible operation: + +```ruby +PrometheusMet.instance.http_requests.increment(labels: { method: 'POST', status: '200' }) +``` + +Do not add new metric names without first checking `lib/helpers/prometheus_metrics.rb` for an existing one that fits. + +--- + +## CI/CD + +The GitHub Actions workflow (`.github/workflows/ruby.yml`) runs on every push/PR: + +1. **Rubocop** — lint check via reviewdog (inline PR comments on violations). +2. **RSpec** — full test suite with a real PostgreSQL 14 service. + +Before opening a PR: + +```bash +bundle exec rubocop # must pass with zero offenses +bundle exec rspec # must pass at ≥90% coverage +rake db:migrate:reset # verify migrations apply cleanly +``` + +--- + +## Common Pitfalls + +- **Do not use `ActiveJob`** — this project uses Delayed Job with plain Ruby workers. +- **Do not call external services from within a Sinatra route** — enqueue a worker instead. +- **Do not modify `db/schema.rb` by hand** — generate a migration. +- **Do not add logic to `app/github_app.rb`** — it should only instantiate and delegate. +- **Do not stub `Time.now` or `Date.today` globally** — use `Timecop` if a test requires time control (not currently a dependency — add it if needed). +- **Do not create new queue numbers** beyond 0–9 without updating `config.ru`. +- **Run `rubocop -A` before committing** — unformatted code fails CI immediately. + +--- + +## Checklist for New Features + +- [ ] New files include the frozen string literal comment and SPDX header. +- [ ] Business logic lives in `lib/`, not in `app/`, `workers/`, or models. +- [ ] New ActiveRecord model includes a migration and status enum (if applicable). +- [ ] New Delayed Job worker lives in `workers/` and delegates to `lib/`. +- [ ] External HTTP calls are wrapped in a class under `lib/bamboo_ci/` or `lib/github/`. +- [ ] Tests mirror the source path and achieve ≥90% branch coverage. +- [ ] WebMock stubs cover all new HTTP calls in tests. +- [ ] `rubocop` passes with no new offenses. +- [ ] `rake db:migrate:reset && bundle exec rspec` passes locally. \ No newline at end of file From 554eaa6dbee210d76946790ed791888aa680e16b Mon Sep 17 00:00:00 2001 From: Rodrigo Nardi Date: Tue, 2 Jun 2026 18:05:15 -0300 Subject: [PATCH 5/5] Refactor PullRequest validations and enhance finished? logic in tests --- lib/models/pull_request.rb | 5 +- spec/lib/github/re_run/comment_spec.rb | 3 +- spec/lib/models/pull_request_spec.rb | 178 ++++++++++++++++++++++--- 3 files changed, 164 insertions(+), 22 deletions(-) diff --git a/lib/models/pull_request.rb b/lib/models/pull_request.rb index 99bafd4..ff1157a 100644 --- a/lib/models/pull_request.rb +++ b/lib/models/pull_request.rb @@ -20,11 +20,12 @@ class PullRequest < ActiveRecord::Base has_many :pull_request_subscriptions, dependent: :delete_all has_many :plans def finished? - return true if check_suites.nil? or check_suites.empty? + last_suite = check_suites.last + return true if check_suites.blank? or last_suite.blank? return plans.all? { |p| current_execution_by_plan(p)&.finished? } if plans.any? - check_suites.last&.finished? || true + last_suite.finished? end def current_execution?(check_suite) diff --git a/spec/lib/github/re_run/comment_spec.rb b/spec/lib/github/re_run/comment_spec.rb index 16e042e..1427aa6 100644 --- a/spec/lib/github/re_run/comment_spec.rb +++ b/spec/lib/github/re_run/comment_spec.rb @@ -312,7 +312,8 @@ let(:payload) do { 'action' => 'created', - 'comment' => { 'body' => "CI:rerun ##{check_suite.commit_sha_ref}", 'id' => 1, 'user' => { 'login' => 'John' } }, + 'comment' => { 'body' => "CI:rerun ##{check_suite.commit_sha_ref}", 'id' => 1, + 'user' => { 'login' => 'John' } }, 'repository' => { 'full_name' => 'test' }, 'issue' => { 'number' => pull_request.github_pr_id } } diff --git a/spec/lib/models/pull_request_spec.rb b/spec/lib/models/pull_request_spec.rb index ddfb9ef..7022ecb 100644 --- a/spec/lib/models/pull_request_spec.rb +++ b/spec/lib/models/pull_request_spec.rb @@ -9,44 +9,184 @@ # frozen_string_literal: true describe PullRequest do - context 'when create a new PR and check if check suite was finished' do + describe 'validations' do + subject { PullRequest.new(author: 'author', github_pr_id: 1, branch_name: 'main', repository: 'org/repo') } + + it 'is valid with all required attributes present' do + expect(subject).to be_valid + end + + it 'is invalid without author' do + subject.author = nil + expect(subject).not_to be_valid + end + + it 'is invalid without github_pr_id' do + subject.github_pr_id = nil + expect(subject).not_to be_valid + end + + it 'is invalid without branch_name' do + subject.branch_name = nil + expect(subject).not_to be_valid + end + + it 'is invalid without repository' do + subject.repository = nil + expect(subject).not_to be_valid + end + end + + describe 'associations' do let(:pull_request) { create(:pull_request) } - it 'must return true' do - expect(pull_request.finished?).to be_truthy + it 'has many check_suites' do + cs = create(:check_suite, pull_request: pull_request) + expect(pull_request.check_suites).to include(cs) + end + + it 'deletes check_suites when the pull request is destroyed' do + create(:check_suite, pull_request: pull_request) + pull_request.plans.delete_all + expect { pull_request.destroy }.to change(CheckSuite, :count).by(-1) + end + + it 'has many pull_request_subscriptions' do + sub = create(:pull_request_subscription, pull_request: pull_request) + expect(pull_request.pull_request_subscriptions).to include(sub) + end + + it 'deletes pull_request_subscriptions when the pull request is destroyed' do + create(:pull_request_subscription, pull_request: pull_request) + pull_request.plans.delete_all + expect { pull_request.destroy }.to change(PullRequestSubscription, :count).by(-1) + end + + it 'has many plans' do + expect(pull_request.plans).not_to be_empty end end - context 'when create a new PR and check if check suite was finished' do - let(:pull_request) { create(:pull_request, :with_check_suite) } + describe '#finished?' do + context 'when check_suites is empty' do + let(:pull_request) { create(:pull_request) } + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'when plans exist and all current executions are finished' do + let(:pull_request) { create(:pull_request) } + let(:plan) { pull_request.plans.first } + + before do + create(:check_suite, pull_request: pull_request, plan: plan) + end + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'when plans exist but no execution is found for a plan' do + let(:pull_request) { create(:pull_request, :with_check_suite) } - it 'must return true' do - expect(pull_request.finished?).to be_falsey + it 'returns false' do + expect(pull_request.finished?).to be false + end + end + + context 'when plans exist and an execution is still running' do + let(:pull_request) { create(:pull_request) } + let(:plan) { pull_request.plans.first } + + before do + cs = create(:check_suite, pull_request: pull_request, plan: plan) + create(:stage, check_suite: cs, status: :in_progress) + end + + it 'returns false' do + expect(pull_request.finished?).to be false + end + end + + context 'when no plans exist and the last check_suite is finished' do + let(:pull_request) { create(:pull_request) } + + before do + pull_request.plans.delete_all + create(:check_suite, pull_request: pull_request) + end + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'when no plans exist and the last check_suite is not finished' do + let(:pull_request) { create(:pull_request) } + + before do + pull_request.plans.delete_all + cs = create(:check_suite, pull_request: pull_request) + create(:stage, check_suite: cs, status: :in_progress) + end + + it 'returns false' do + expect(pull_request.finished?).to be_falsey + end end end - context 'when current execution is not the last check suite' do + describe '#current_execution?' do let(:pull_request) { create(:pull_request) } - let(:check_suite1) { create(:check_suite, pull_request: pull_request) } - let(:check_suite2) { create(:check_suite, pull_request: pull_request) } - let(:check_suite3) { create(:check_suite, pull_request: pull_request) } + let(:cs1) { create(:check_suite, pull_request: pull_request) } + let(:cs2) { create(:check_suite, pull_request: pull_request) } before do - check_suite1 - check_suite2 - check_suite3 + cs1 + cs2 + end + + it 'returns true when the check_suite is the latest execution for its plan' do + expect(pull_request.current_execution?(cs2)).to be true end - it 'must return true' do - expect(pull_request.current_execution?(check_suite3)).to be_truthy + it 'returns false when the check_suite is not the latest execution for its plan' do + expect(pull_request.current_execution?(cs1)).to be false end end - context 'when current execution is nil' do + describe '#current_execution_by_plan' do let(:pull_request) { create(:pull_request) } + let(:plan) { pull_request.plans.first } + + context 'when check_suites exist for the given plan' do + let!(:cs1) { create(:check_suite, pull_request: pull_request, plan: plan) } + let!(:cs2) { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns the last check_suite ordered by id' do + expect(pull_request.current_execution_by_plan(plan)).to eq(cs2) + end + end + + context 'when no check_suites exist for the given plan' do + it 'returns nil' do + expect(pull_request.current_execution_by_plan(plan)).to be_nil + end + end + end + + describe '.unique_repository_names' do + before do + create(:pull_request, repository: 'org/repo-a') + create(:pull_request, repository: 'org/repo-a') + create(:pull_request, repository: 'org/repo-b') + end - it 'must return true in finished?' do - expect(pull_request.finished?).to be_truthy + it 'returns distinct repository names without duplicates' do + expect(PullRequest.unique_repository_names).to contain_exactly('org/repo-a', 'org/repo-b') end end end