fix(ci): require SDK Value table in FINAL reviews and extend TS merge-gate parity - #4531
Conversation
…-gate parity FINAL architecture reviewer prompt now mandates the Phase 1 table with an explicit SDK value row and TS AGENTS.md routing. Merge-gate helpers treat src/praisonai-ts/ as SDK scope for test enforcement and CI check patterns. Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe merge gate now recognizes the TypeScript SDK, requires matching TypeScript tests for SDK source changes, includes TypeScript checks, and directs Claude reviews to TypeScript-specific guidance and SDK value criteria. ChangesTypeScript SDK merge-gate support
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR expands merge-gate policy and review prompts to cover the TypeScript SDK and require an SDK-value table. It also adds TypeScript missing-test heuristics, CI check patterns, and self-tests, but the new enforcement paths can accept unrelated evidence and the table remains prompt-only.
Confidence Score: 2/5The PR should not merge until TypeScript checks and test files are scoped to the SDK they validate and the claimed mandatory review table is actually enforced. Unrelated test files and Python-only checks can satisfy the newly extended TypeScript gates, while an omitted SDK-value table can still lead to an accepted approval verdict. Files Needing Attention: .github/scripts/merge-gate.js, .github/scripts/merge-gate-selftest.js, .github/workflows/claude-merge-gate.yml
|
| Filename | Overview |
|---|---|
| .github/scripts/merge-gate.js | Expands SDK scope and review requirements, but test-file and CI-check matching can accept unrelated coverage and the mandatory table is not parsed. |
| .github/scripts/merge-gate-selftest.js | Adds basic TypeScript and prompt-content assertions but omits mixed-scope tests and behavior-level validation of the mandatory table. |
| .github/workflows/claude-merge-gate.yml | Extends the assessor prompt to TypeScript and the SDK-value table, while continuing to enforce only the assessor's verdict marker deterministically. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SDK PR] --> B[missingTestsReason]
A --> C[sdkTestChecksReason]
A --> D[FINAL review trigger]
B --> E{Relevant test file?}
C --> F{Matching check name?}
D --> G[LLM review response]
G --> H[Assessor verdict]
E --> I[evaluatePipelineQuiescent]
F --> I
H --> J[Merge decision]
I --> J
Reviews (1): Last reviewed commit: "fix(ci): require SDK Value table in FINA..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const hasPyTestChange = files.some( | ||
| (f) => /\/tests?\//.test(f.filename) || /test_.*\.py$/.test(f.filename) || /_test\.py$/.test(f.filename) | ||
| ); | ||
| if (!hasTestChange) return 'SDK code added without test file changes — requires manual review'; | ||
| const hasTsTestChange = files.some( | ||
| (f) => | ||
| f.filename.startsWith('src/praisonai-ts/tests/') || | ||
| /\.(test|spec)\.ts$/.test(f.filename) | ||
| ); |
There was a problem hiding this comment.
When a PR adds Python SDK code alongside a TypeScript test, or TypeScript SDK code alongside an out-of-package .test.ts or .spec.ts file, these repository-wide predicates treat the unrelated test as SDK coverage, allowing untested SDK changes to pass the missing-tests gate.
| const WORKFLOW_ONLY_LABEL = 'merge-gate-ci-only'; | ||
| const CI_ONLY_PATH_PREFIXES = ['.github/workflows/', '.github/actions/', '.github/scripts/merge-gate']; | ||
| const SDK_PATH_PREFIXES = ['src/praisonai-agents/', 'src/praisonai/']; | ||
| const SDK_PATH_PREFIXES = ['src/praisonai-agents/', 'src/praisonai/', 'src/praisonai-ts/']; |
There was a problem hiding this comment.
When a TypeScript-only SDK PR triggers the Python test-core workflow, the new TypeScript SDK prefix activates sdkTestChecksReason and the existing /core/i pattern accepts that Python-only check, allowing the merge gate to report TypeScript CI coverage even though no TypeScript tests ran.
| const FINAL_CLAUDE_REVIEW_BODY = | ||
| '@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.\n\n**Phase 1: Review per AGENTS.md**\n1. Protocol-driven: check heavy implementations vs core SDK\n2. Backward compatible: ensure zero feature regressions\n3. Performance: no hot-path regressions\n4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep\n5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.\n6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (`praisonai.sandbox` entry point) — request changes if wrongly added to praisonaiagents/\n\n**Phase 2: FIX Valid Issues**\n7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix\n8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback\n9. Push all code fixes directly to THIS branch (do NOT create a new PR)\n10. Comment a summary of exact files modified and what you skipped\n\n**Phase 3: Final Verdict**\n11. If all issues are resolved, approve the PR / close the Issue\n12. If blocking issues remain, request changes / leave clear action items'; | ||
| '@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.\n\n**MANDATORY READ (before reviewing):**\n- Always read src/praisonai-agents/AGENTS.md\n- If this PR touches src/praisonai-ts/, also read src/praisonai-ts/AGENTS.md §2.1.2 (TS triage + PR review checklist)\n\n**Phase 1: Review per AGENTS.md**\n1. Protocol-driven: check heavy implementations vs core SDK\n2. Backward compatible: ensure zero feature regressions\n3. Performance: no hot-path regressions\n4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep\n5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.\n6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (`praisonai.sandbox` entry point) — request changes if wrongly added to praisonaiagents/\n\n**MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment:**\n#### Phase 1 — AGENTS.md review\n| Check | Result |\n|---|---|\n| Protocol-driven / no heavy impl in core | ✅ or ❌ + one-line rationale |\n| Backward compatible | ✅ or ❌ + one-line rationale |\n| Performance (hot path) | ✅ or ❌ + one-line rationale |\n| **SDK value** | ✅ or ❌ + one-line rationale (explicitly judge whether the change strengthens the SDK) |\n| No Agent param bloat | ✅ or ❌ + one-line rationale |\n| Repo routing | ✅ or ❌ + one-line rationale |\n\nFor TypeScript PRs (src/praisonai-ts/), also add:\n| TS types / parity / tests | ✅ or ❌ + one-line rationale (npm run build && npm test) |\n\n**Phase 2: FIX Valid Issues**\n7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix\n8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback\n9. Push all code fixes directly to THIS branch (do NOT create a new PR)\n10. Comment a summary of exact files modified and what you skipped\n\n**Phase 3: Final Verdict**\n11. If all issues are resolved, approve the PR / close the Issue\n12. If blocking issues remain, request changes / leave clear action items'; |
There was a problem hiding this comment.
When a FINAL review omits or malforms the Phase 1 table and the assessor still posts MERGE_GATE_VERDICT: APPROVE, completion and verdict handling never inspect the FINAL response body, so the PR can pass without the SDK-value row this change requires.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. MANDATORY READ (before reviewing):
Phase 1: Review per AGENTS.md
MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment: Phase 1 — AGENTS.md review
For TypeScript PRs (src/praisonai-ts/), also add: Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job • FINAL Architecture Review — PR #4531
Phase 1 — AGENTS.md review
(No Phase 2 — Reviewer feedback triage & fixesThree Greptile P1 findings reviewed:
Files modified (pushed to
Self-tests: Phase 3 — VerdictApprove (original PR) with follow-up fixes applied. The two exploitable cross-SDK bypasses (P1 #1, #2) are resolved and covered by new self-tests. The unenforced-table concern (P1 #3) is intentionally deferred as a design item — the prompt still mandates the table, and hard body-parsing enforcement should be scoped separately to avoid false BLOCKs. Since #4531 is already merged, changes landed on a new branch: Job run · branch |
Summary
FINAL_CLAUDE_REVIEW_BODY, so FINAL architecture reviews are consistent for Python, TypeScript, and other PRs — not ad-hoc prose.src/praisonai-ts/AGENTS.md §2.1.2and adds a TS-specific table row (TS types / parity / tests).src/praisonai-ts/(test enforcement, CI check patterns) and updates the merge-gate assessor prompt to cover both Python and TypeScript SDKs.Root cause
FINAL
@claudereviews had SDK value as a bullet requirement but no mandatory output format, so Claude sometimes included the table (e.g. #4513) and sometimes skipped it (e.g. #4505, #4520). Merge-gate logic was also Python-only (SDK_PATH_PREFIXES,missingTestsReason, assessor prompt).Test plan
node .github/scripts/merge-gate-selftest.js— 52/52 pass@claudeon an openpraisonai-tsPR and confirm Phase 1 table includes SDK value rowMade with Cursor
Summary by CodeRabbit