-
Notifications
You must be signed in to change notification settings - Fork 41
feat: Dynamic reviews, context batching, AWS Bedrock; GHES preserved; CLI+tests #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… CLI+tests Dynamic Reviews Add custom prompt mode with customMode: on | off | auto Auto mode routes complex code files to custom prompts Capture documentation from first custom review batch and include in overview Context Batching Batch PR files by character size to respect config.maxReviewChars (default 725k) Aggregate comments across batches AWS Bedrock + SAP AI Core Support Bedrock with IAM credentials (no LLM_API_KEY required when AWS creds present and model is bedrock/anthropic/meta/amazon family) Keep SAP AI Core path (no API key required with proper SAP vars) GitHub Enterprise Server (GHES) preserved Inputs: github_api_url, github_server_url Pass baseUrl to Octokit via initOctokit(config.githubToken, config.githubApiUrl) CLI Add --full (force full review) and --out (save output to file) flags Comments improvements Truncate code blocks in comments using config.maxCodeblockLines with marker: “… (truncated; more lines omitted) …” Support multiline threads (start_line) in generateCommentThreads Paginate listReviewComments to fetch all pages Providers AI SDK default temperature now 1 (when undefined) Action + Config action.yml: added inputs for new config (custom_mode, llm_provider, llm_model, review_scopes, allow_title_update, max_comments, max_codeblock_lines, max_review_chars) branding: restored tags per request (ai, code-review, ai-assisted, automation, pull-requests, linting, ci, autofix, devtools) Entry point remains dist/index.js Build Revert build script to two explicit esbuild outputs (dist/index.js, dist/cli.js) Tests and Mocks All tests green: 56 passed Updated @octokit/action mock to expose Octokit.__lastOptions for throttle tests Avoid custom prompt runner during tests to align with mocks Removals Removed push.ts and its tests per requirement Misc messages.ts: overview accepts optional documentation section
|
⏳ Analyzing changes in this PR... ⏳ This might take a few minutes, please wait 📥 CommitsAnalyzing changes from base (
Replaces legacy dotenv import and usage in dist/cli.js and related files with a new import style and updated variable names. This change improves consistency and maintainability by aligning with the latest build output and source conventions.
Introduces the optional LLM_BASE_URL environment variable for OpenAI-compatible providers when using LLM_PROVIDER=ai-sdk. Updates documentation and configuration to clarify usage, enabling support for providers like OpenRouter, Anyscale, and Together AI. No changes for sap-ai-sdk provider.
The action now allows PR title updates by default when @presubmit is present, updating the default in both action.yml and config.ts. The unused max_comments input and related logic have been removed for simplification.
type safety + schema validation...
Dynamic Reviews 📁 Files being considered (23)🔄 README.md (2 hunks) autogenerated by presubmit.ai |
|
Great PR! Do you have an example of a review with your changes applied? |
bstanga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to rebase
src/comments.ts
Outdated
| if (isFence(line)) { | ||
| if (inBlock) { | ||
| // closing fence | ||
| if (emittedTrunc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this if statement, it's no-op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 4741885: build
Files Processed (2)
- dist/cli.js (0 hunks)
- dist/index.js (0 hunks)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Files Processed (3)
- dist/cli.js (0 hunks)
- dist/index.js (0 hunks)
- src/prompts.ts (1 hunk)
Actionable Comments (0)
Skipped Comments (4)
-
src/prompts.ts [311-315]
best practice: "Type annotation missing for raw variable"
-
src/prompts.ts [320-322]
possible issue: "Hardcoded parameter name may not match all providers"
-
src/prompts.ts [325-327]
maintainability: "Error message could be more informative"
-
src/prompts.ts [329-339]
maintainability: "Default values may mask provider issues"
…nd @actions/github version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 7afa451: bump github
Files Processed (3)
- dist/cli.js (0 hunks)
- dist/index.js (0 hunks)
- package.json (1 hunk)
Actionable Comments (0)
Skipped Comments (0)
|
Okay, again, maybe confirm... sorry for the issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 7017eaa: Update README.md
Files Processed (1)
- README.md (2 hunks)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM!
Review Summary
Commits Considered (1)
- 0a76989: Update README.md
Files Processed (1)
- README.md (2 hunks)
Actionable Comments (0)
Skipped Comments (0)
|
Sorry for delay, I've been on an extended leave. Will review this asap next week |
The action now allows PR title updates by default when @presubmit is present, updating the default in both action.yml and config.ts. The unused max_comments input and related logic have been removed for simplification.
Introduces the optional LLM_BASE_URL environment variable for OpenAI-compatible providers when using LLM_PROVIDER=ai-sdk. Updates documentation and configuration to clarify usage, enabling support for providers like OpenRouter, Anyscale, and Together AI. No changes for sap-ai-sdk provider.
|
@bstanga fixed both issues and merged your changes in from main |
Replaces legacy dotenv import and usage in dist/cli.js and related files with a new import style and updated variable names. This change improves consistency and maintainability by aligning with the latest build output and source conventions.
Dynamic Reviews
Add custom prompt mode with customMode: on | off | auto
Auto mode routes complex code files to custom prompts
Capture documentation from first custom review batch and include in overview
Context Batching
Batch PR files by character size to respect config.maxReviewChars (default 725k)
Aggregate comments across batches
AWS Bedrock + SAP AI Core
Support Bedrock with IAM credentials (no LLM_API_KEY required when AWS creds present and model is bedrock/anthropic/meta/amazon family)
Keep SAP AI Core path (no API key required with proper SAP vars)
GitHub Enterprise Server (GHES) preserved
Inputs: github_api_url, github_server_url
Pass baseUrl to Octokit via initOctokit(config.githubToken, config.githubApiUrl)
CLI
Add --full (force full review) and --out (save output to file) flags
Comments improvements
Truncate code blocks in comments using config.maxCodeblockLines with marker: “… (truncated; more lines omitted) …”
Support multiline threads (start_line) in generateCommentThreads
Paginate listReviewComments to fetch all pages
Providers
AI SDK default temperature now 1 (when undefined)
Action + Config
action.yml: added inputs for new config (custom_mode, llm_provider, llm_model, review_scopes, allow_title_update, max_comments, max_codeblock_lines, max_review_chars)
branding: restored tags per request (ai, code-review, ai-assisted, automation, pull-requests, linting, ci, autofix, devtools)
Entry point remains dist/index.js
Build
Revert build script to two explicit esbuild outputs (dist/index.js, dist/cli.js)
Tests and Mocks
All tests green: 56 passed
Updated @octokit/action mock to expose Octokit.__lastOptions for throttle tests
Avoid custom prompt runner during tests to align with mocks
Misc
messages.ts: overview accepts optional documentation section