Skip to content

[QQ-1736] feature/migration-detector#32

Merged
zejuniortdr merged 14 commits into
mainfrom
feature/QQ-1736-migration-detector
Mar 5, 2026
Merged

[QQ-1736] feature/migration-detector#32
zejuniortdr merged 14 commits into
mainfrom
feature/QQ-1736-migration-detector

Conversation

@zejuniortdr

Copy link
Copy Markdown
Collaborator

Ticket

Automatizar a notificação para o time de dados sempre que houver uma migração no banco.

Descrição

Após alguns alinhamentos com o time de dados e uma das dores que eles trouxeram é com relação a eventuais mudanças que a gente possa fazer que acabe quebrando do lado deles.

Há um tempo existiu uma iniciativa de automatizar no Jira essa comunicação, mas que acabou não indo tão pra frente.

Visando aumentar a integração entre tech e dados, esse card visa trazer uma proposta para notificação automática sempre que houver alguma migração no banco, considerando os cenários:

Safe Change

Descrição
Adição da coluna qty na tabela database.table.

Exemplos:

  • Adicionar coluna opcional
  • Adicionar nova tabela não consumida
  • Adicionar novo valor enum com fallback
  • Adicionar índice
  • Adicionar campo novo na camada analítica

Mudança Controlada

Descrição
Alteração de VARCHAR (30) para VARCHAR (255) do campo qty na tabela database.table

Exemplos:

  • Alterar tamanho de varchar
  • Alterar precisão numérica
  • Tornar campo nullable
  • Alterar valor default

Breaking Change

Descrição
Remoção do campo qty na tabela database.table

Exemplos:

  • Remover campo
  • Renomear campo
  • Alterar tipo de dado
  • Alterar chave primária
  • Remoção de tabela consumida

@zejuniortdr zejuniortdr self-assigned this Feb 25, 2026
@github-actions github-actions Bot added the waiting review Waiting 2 approvals label Feb 25, 2026
@zejuniortdr zejuniortdr changed the title [QQ-1736] Feature/ migration detector [QQ-1736] Feature/migration-detector Feb 25, 2026
@zejuniortdr zejuniortdr changed the title [QQ-1736] Feature/migration-detector [QQ-1736] feature/migration-detector Feb 25, 2026

@wedneyyuri wedneyyuri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bom trabalho no conceito e na spec do README! A implementação está bem encaminhada, mas encontrei alguns pontos que precisam de atenção antes do merge — especialmente o fail_on_breaking que não tem efeito no código atual.

Resumo:

  • 🔴 2 issues críticas
  • 🟠 4 issues altas
  • 🟡 6 issues médias
  • 🟢 2 issues baixas

Pontos positivos:

  • README excelente com spec clara, guia de setup e regras de governança
  • Boa estratégia de fallback quando IA falha (default para controlled)
  • Promoção de confiança inteligente (safe → controlled quando baixa confiança)
  • Formatação limpa da mensagem Slack com cores por severidade
  • Suporte flexível de providers (GitHub Models vs hub externo)

Comment thread migration-detector/action.yml Outdated
Comment thread migration-detector/action.yml Outdated
Comment thread migration-detector/action.yml Outdated
Comment thread migration-detector/action.yml Outdated
Comment thread migration-detector/action.yml
Comment thread migration-detector/action.yml Outdated
Comment thread migration-detector/action.yml Outdated
Comment thread migration-detector/action.yml
Comment thread migration-detector/action.yml Outdated
Comment thread .gitignore Outdated
zejuniortdr and others added 2 commits February 26, 2026 11:01
Security fixes:
- Replace grep -Eiv with grep -Fiv to prevent command injection
- Remove SLACK_WEBHOOK_URL from collect step env, use boolean flag
- Use curl --data @- to pipe JSON payload via stdin

Code quality improvements:
- Add set -euo pipefail to all multi-line shell blocks
- Fix mutation patterns with immutable spread syntax
- Add retry logic (2 retries, 2s sleep) for AI API calls
- Add warning log when file truncation occurs
- Add env var validation with user-friendly error messages

Refactoring:
- Split requirements.txt into runtime and dev dependencies
- Remove undocumented "MO" fallback key from build_slack_text
- Replace lambda assignment with def for PEP 8 compliance
- Update tests to reflect new behavior

All 63 tests pass.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@wedneyyuri

Copy link
Copy Markdown
Contributor

Code Review Fixes Applied

I've reviewed PR #32 and applied fixes for 13 security and code quality issues across 7 files. All changes have been committed in ae8ac14.

Summary of Changes

🔒 Security Fixes (CRITICAL/HIGH)

  1. Command injection prevention — Changed grep -Eiv "$term" to grep -Fiv "$term" in action.yml:156 to prevent regex metacharacter injection via user-supplied ignore terms
  2. Secret exposure — Removed SLACK_WEBHOOK_URL from the collect step's environment (replaced with HAS_SLACK_WEBHOOK boolean flag). The webhook secret is now only present in the step that actually uses it
  3. Defense-in-depth curl usage — Changed from --data "$PAYLOAD" to --data @- with stdin piping in action.yml:286-293, eliminating shell interpolation of JSON content

✅ Code Quality Improvements (HIGH/MEDIUM)

  1. Error handling — Added set -euo pipefail to all 7 multi-line shell run: blocks for proper error propagation
  2. Immutability — Fixed mutation patterns in classify.py:290 and apply_confidence_threshold:169 using immutable spread syntax {**dict, key: val}
  3. AI retry logic — Added 2-retry loop with 2s sleep in classify.py:265-278 to handle transient API failures before falling back to conservative result
  4. File truncation warning — Added stderr warning in classify.py:76-79 when migration files are truncated at 6KB limit
  5. Env var validation — Added SystemExit with user-friendly error messages in classify.py:241-244 for missing AI_API_URL/AI_MODEL
  6. Git diff error check — Changed from unreliable $? pattern to if ! CHANGED=$(...) in action.yml:102 for compatibility with set -e

🧹 Refactoring (MEDIUM/LOW)

  1. Dependency separation — Split requirements.txt into runtime (openai==2.24.0) and dev (requirements-dev.txt with pytest>=8.0) to avoid installing test deps in production
  2. Removed legacy code — Deleted undocumented "MO" fallback key from build_slack_text:187 in classify.py
  3. Immutable dict construction — Refactored build_slack_payload.py:9-22 to use conditional spread **({"channel": channel} if channel else {}) instead of post-construction mutation
  4. PEP 8 compliance — Changed lambda assignment to def in test_build_slack_payload.py:15

Verification

✅ All 63 tests passing
✅ Final review completed — no blocking issues remaining
✅ Ready to merge

Files Modified

  • migration-detector/action.yml (+21/-13 lines)
  • migration-detector/classify.py (+27/-16 lines)
  • migration-detector/build_slack_payload.py (+1/-7 lines)
  • migration-detector/requirements.txt (-1 line)
  • migration-detector/requirements-dev.txt (new file, +2 lines)
  • migration-detector/tests/test_build_slack_payload.py (+2/-1 lines)
  • migration-detector/tests/test_classify.py (+4/-1 lines)

The code follows all security best practices, uses immutable patterns throughout, and has comprehensive test coverage. 🚀

wedneyyuri
wedneyyuri previously approved these changes Feb 26, 2026
Comment thread migration-detector/classify.py Outdated
LFelipe-sb
LFelipe-sb previously approved these changes Feb 26, 2026

@LFelipe-sb LFelipe-sb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚀
Fiz uma sugestão, mas vou deixar aprovado caso avalie não ser necessário.

@github-actions github-actions Bot removed the waiting review Waiting 2 approvals label Feb 26, 2026
@wedneyyuri wedneyyuri dismissed stale reviews from LFelipe-sb and themself via ae8ac14 February 26, 2026 22:08
@github-actions github-actions Bot added waiting review Waiting 2 approvals and removed waiting QA labels Feb 26, 2026
@zejuniortdr zejuniortdr requested review from a team and LFelipe-sb February 27, 2026 12:51

@gleberdiniz gleberdiniz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@gleberdiniz

Copy link
Copy Markdown
Contributor

Talvez uma melhoria para projetos django seria rodar esse cara aqui antes, não teria custo da IA:
https://github.com/YasserShkeir/django-safe-migrations

@zejuniortdr

Copy link
Copy Markdown
Collaborator Author

https://github.com/YasserShkeir/django-safe-migrations

@gleberdiniz , acho excelente conseguimos colocar isso nos projetos que usam o Django até para reduzir o custo com IA. Olhando a doc, achei bem completo os erros mapeados e que podemos fazer para uma futura melhoria é ajustar o prompt aqui para seguir estas regras também.

Como essa actions além de classificar, também fará um resumo do que está sendo alterado, principalmente para contexto do time de dados, acho que não seria escopo desse repo, mas sim dos que utilizam o framework, o que acha?

@github-actions github-actions Bot added waiting QA and removed waiting review Waiting 2 approvals labels Mar 2, 2026

@RafaelMascarenhasC RafaelMascarenhasC left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TOP DEMAIS !!! 🚀

@zejuniortdr zejuniortdr merged commit c133189 into main Mar 5, 2026
7 of 8 checks passed
@zejuniortdr zejuniortdr deleted the feature/QQ-1736-migration-detector branch March 5, 2026 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants