-
Notifications
You must be signed in to change notification settings - Fork 155
Bug/ai bot reponse correctly #251
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
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe PR removes configuration placeholder files, refactors debate bot prompt handling to split single prompts into system and user components with response validation and retry mechanisms, extends Gemini text generation to support system instructions, updates related service calls to accommodate the new parameter structure, and reduces minimum password length from 8 to 6 characters across backend and frontend. Changes
Sequence DiagramsequenceDiagram
participant Client as Bot Service<br/>(debatevsbot.go)
participant Prompt as Prompt<br/>Constructor
participant Gemini as Gemini<br/>Service
participant Validator as Response<br/>Validator
Client->>Prompt: Construct system & user prompts
Prompt->>Prompt: Build CORE RULES, PERSONA,<br/>PHASE info
Prompt-->>Client: Return (systemPrompt, userPrompt)
Client->>Gemini: generateModelText(ctx,<br/>systemPrompt, userPrompt)
Gemini->>Gemini: Apply system instruction<br/>to API config
Gemini-->>Client: AI response
Client->>Validator: validateResponse(response)
alt Response Valid
Validator-->>Client: Pass validation
Client-->>Client: Return final response
else Response Invalid
Validator-->>Client: Fail validation
Client->>Prompt: Enhance systemPrompt<br/>with reminder
Client->>Gemini: Retry with<br/>updated systemPrompt
Gemini-->>Client: Revised response
Client-->>Client: Return response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/gemini.go (1)
21-47: UnusedmodelNameparameter ingenerateModelText.The function accepts
modelNameas a parameter (line 21), but line 43 always uses thedefaultGeminiModelconstant instead. Either use the passedmodelNameparameter or remove it from the function signature.Proposed fix
- resp, err := geminiClient.Models.GenerateContent(ctx, defaultGeminiModel, genai.Text(prompt), config) + resp, err := geminiClient.Models.GenerateContent(ctx, modelName, genai.Text(prompt), config)
🤖 Fix all issues with AI agents
In `@backend/services/debatevsbot.go`:
- Around line 178-194: validateResponse currently only checks length and a
greeting, so implement a minimal topical/argumentative check using the provided
topic and a small list of argumentative cues: extract lowercased keyword tokens
from the topic (split on non-word chars) and check if the response contains any
topic keywords OR any argumentative words (e.g., "because", "however",
"therefore", "argue", "claim", "evidence", "but"); if neither is present return
false. Update validateResponse to use the topic parameter and the
argumentativeWords slice to decide validity, ensuring you normalize case and
check word boundaries (use strings.Contains or simple token comparisons) so
off-topic/generic replies are rejected and retries will trigger.
In `@backend/structs/auth.go`:
- Line 5: The Password struct field validation was weakened to min=6; revert it
to min=8 to restore previous backend password policy. Update the Password tag on
the struct (the Password field in backend/structs/auth.go) to use
binding:"required,min=8", and ensure any related server-side validation logic or
unit tests that reference the minimum length (e.g., auth validation routines or
tests) are updated to expect 8 characters so backend and frontend remain
consistent with the stronger policy.
In `@frontend/src/Pages/Authentication/forms.tsx`:
- Around line 8-9: MIN_PASSWORD_LENGTH set to 6 is too low; update the constant
MIN_PASSWORD_LENGTH in forms.tsx to a stronger minimum (at least 8 if MFA is
used, preferably 12) and adjust any related validation logic, user-facing
messages, and tests that reference MIN_PASSWORD_LENGTH (e.g., password
validation functions and form error strings) so they enforce and communicate the
new minimum.
🧹 Nitpick comments (2)
frontend/src/Pages/Authentication/forms.tsx (1)
29-37: Inconsistent error handling pattern across forms.
LoginFormuseslocalErrorstate (line 32) whileSignUpFormandResetPasswordFormuseauthContext.handleError. Consider using a consistent approach across all forms for maintainability.backend/services/debatevsbot.go (1)
123-143: EnsureCURRENT PHASEnever ends up blank.If the last user message has an empty
Phase, the prompt prints a blank phase and falls through to the default branch. A small fallback keeps the prompt consistent and avoids confusing the model.🔧 Suggested adjustment
currentPhase := "Opening Statement" if len(history) > 1 { lastMsg := findLastUserMessage(history) - currentPhase = lastMsg.Phase - if strings.ToLower(currentPhase) == "first rebuttal" || strings.ToLower(currentPhase) == "second rebuttal" { + currentPhase = strings.TrimSpace(lastMsg.Phase) + if currentPhase == "" { + currentPhase = "Unspecified Phase" + } + if strings.ToLower(currentPhase) == "first rebuttal" || strings.ToLower(currentPhase) == "second rebuttal" { currentPhase = "Cross Examination" } }
| // validateResponse performs basic checks on the generated text. | ||
| func validateResponse(text string, topic string) bool { | ||
| lowerText := strings.ToLower(text) | ||
| // Reject very short filler | ||
| if len(strings.Fields(text)) < 5 { | ||
| return false | ||
| } | ||
| // Reject obvious casual filler if it's the only thing there | ||
| casualFiller := []uintptr{'h', 'e', 'l', 'l', 'o'} | ||
| _ = casualFiller // just avoiding unused | ||
| if strings.Contains(lowerText, "hello there") && len(text) < 30 { | ||
| return false | ||
| } | ||
| // Check for a few "argumentative" words or topic keywords (this is a weak check but better than nothing) | ||
| // In a real scenario, we might want deeper NLP or keyword matching against the topic. | ||
| return true | ||
| } |
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.
validateResponse doesn’t actually validate topical/argumentative content.
Right now it only rejects very short text (and one greeting), so generic or off-topic replies pass and the retry almost never triggers. This undermines the new “retry on weak response” behavior.
✅ Minimal topical/argumentative check
func validateResponse(text string, topic string) bool {
lowerText := strings.ToLower(text)
// Reject very short filler
if len(strings.Fields(text)) < 5 {
return false
}
// Reject obvious casual filler if it's the only thing there
- casualFiller := []uintptr{'h', 'e', 'l', 'l', 'o'}
- _ = casualFiller // just avoiding unused
if strings.Contains(lowerText, "hello there") && len(text) < 30 {
return false
}
- // Check for a few "argumentative" words or topic keywords (this is a weak check but better than nothing)
- // In a real scenario, we might want deeper NLP or keyword matching against the topic.
- return true
+ // Basic topical/argumentative signal (weak but better than nothing)
+ topicTokens := strings.Fields(strings.ToLower(topic))
+ hasTopic := false
+ for _, t := range topicTokens {
+ t = strings.Trim(t, ".,!?\"'():;")
+ if len(t) < 3 {
+ continue
+ }
+ if strings.Contains(lowerText, t) {
+ hasTopic = true
+ break
+ }
+ }
+ argumentative := []string{"because", "therefore", "since", "evidence", "reason", "argue", "claim", "thus"}
+ hasArgument := false
+ for _, w := range argumentative {
+ if strings.Contains(lowerText, w) {
+ hasArgument = true
+ break
+ }
+ }
+ return hasTopic || hasArgument
}🤖 Prompt for AI Agents
In `@backend/services/debatevsbot.go` around lines 178 - 194, validateResponse
currently only checks length and a greeting, so implement a minimal
topical/argumentative check using the provided topic and a small list of
argumentative cues: extract lowercased keyword tokens from the topic (split on
non-word chars) and check if the response contains any topic keywords OR any
argumentative words (e.g., "because", "however", "therefore", "argue", "claim",
"evidence", "but"); if neither is present return false. Update validateResponse
to use the topic parameter and the argumentativeWords slice to decide validity,
ensuring you normalize case and check word boundaries (use strings.Contains or
simple token comparisons) so off-topic/generic replies are rejected and retries
will trigger.
| type SignUpRequest struct { | ||
| Email string `json:"email" binding:"required,email"` | ||
| Password string `json:"password" binding:"required,min=8"` | ||
| Password string `json:"password" binding:"required,min=6"` |
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.
Backend password minimum reduced to 6 characters - security concern.
As noted in the frontend review, reducing the minimum password length from 8 to 6 characters weakens security. While frontend and backend are now consistent, this change affects all authentication flows. Consider reverting to min=8 unless there's a documented business requirement.
🤖 Prompt for AI Agents
In `@backend/structs/auth.go` at line 5, The Password struct field validation was
weakened to min=6; revert it to min=8 to restore previous backend password
policy. Update the Password tag on the struct (the Password field in
backend/structs/auth.go) to use binding:"required,min=8", and ensure any related
server-side validation logic or unit tests that reference the minimum length
(e.g., auth validation routines or tests) are updated to expect 8 characters so
backend and frontend remain consistent with the stronger policy.
| const MIN_PASSWORD_LENGTH = 6; | ||
|
|
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.
🧩 Analysis chain
🏁 Script executed:
# Check the actual password length constant in the file
cd frontend/src/Pages/Authentication && cat -n forms.tsx | head -20Repository: AOSSIE-Org/DebateAI
Length of output: 870
🏁 Script executed:
# Look for git history to confirm previous password length
git log --oneline -n 10 -- frontend/src/Pages/Authentication/forms.tsx 2>/dev/null || echo "Git history not available"Repository: AOSSIE-Org/DebateAI
Length of output: 97
🏁 Script executed:
# Search for all MIN_PASSWORD_LENGTH usages in the file
rg "MIN_PASSWORD_LENGTH" frontend/src/Pages/Authentication/forms.tsx -B 2 -A 2Repository: AOSSIE-Org/DebateAI
Length of output: 779
🌐 Web query:
OWASP password length requirements 2024 2025
💡 Result:
Short summary of OWASP guidance (2024–2025):
- Minimums: OWASP’s Authentication guidance references NIST: if MFA is enabled, passwords <8 chars are weak; if MFA is not enabled, passwords <15 chars are weak. ASVS (OWASP verification standard) specifies passwords are at least 12 characters. [1][2][3]
- Maximums and passphrases: Allow long passwords/passphrases — OWASP recommends supporting at least 64 characters (many OWASP pages permit up to 128 or more and warn against low maxima). Do not silently truncate; allow spaces and Unicode. [1][2]
- Storage notes: When using bcrypt note its 72‑byte input limit (or pre‑hash safely); prefer Argon2id/scrypt and choose appropriate max/input handling. [2][4]
Sources:
- OWASP Authentication Cheat Sheet. [1]
- OWASP Password Storage Cheat Sheet. [2][4]
- OWASP ASVS — Password Security Requirements (2.1). [3]
🌐 Web query:
NIST SP 800-63B password minimum length requirements 2024
💡 Result:
Short answer — current NIST SP 800-63B requirements (as published on the NIST 800-63 site):
- Passwords used as a single-factor SHALL be at least 15 characters long. [1]
- Passwords used only as part of a multi‑factor authentication MAY be shorter but SHALL be at least 8 characters. [1]
- Verifiers SHOULD permit a maximum password length of at least 64 characters and SHOULD accept all printing ASCII (including space) and Unicode characters; composition rules (e.g., requiring symbols/uppercase) SHALL NOT be imposed. [2]
Sources: NIST SP 800-63B (authenticator and password verifier requirements). [1][2]
Minimum password length of 6 characters is below current security standards.
Current OWASP ASVS requires minimum 12 characters. NIST SP 800-63B specifies minimum 8 characters for multi-factor authentication and minimum 15 characters for single-factor authentication. At 6 characters, this falls short of all current industry guidelines and should be increased to at least 8 characters (if MFA is used) or preferably 12 characters.
🤖 Prompt for AI Agents
In `@frontend/src/Pages/Authentication/forms.tsx` around lines 8 - 9,
MIN_PASSWORD_LENGTH set to 6 is too low; update the constant MIN_PASSWORD_LENGTH
in forms.tsx to a stronger minimum (at least 8 if MFA is used, preferably 12)
and adjust any related validation logic, user-facing messages, and tests that
reference MIN_PASSWORD_LENGTH (e.g., password validation functions and form
error strings) so they enforce and communicate the new minimum.
Fix: Enforce Debate Constraints for AI Debater Responses
Motivation
During User-vs-Bot debates, the AI debater occasionally produced casual or irrelevant filler responses, particularly in the Opening Statements phase. This behavior undermines DebateAI’s core objective of facilitating structured, competitive debates.
Root cause analysis showed that debate rules (topic, stance, phase) and persona flair were combined in a single prompt, allowing persona tone to override debate constraints.
What This PR Does
This PR introduces a clear separation between system-level debate rules and user-level context, ensuring the AI debater remains on-topic and argumentative across all debate phases.
Technical Changes
1. Gemini Service Enhancement
Files:
gemini.gogenerateModelTextandgenerateDefaultModelTextto accept an optionalsystemInstruction.genai.GenerateContentConfig.SystemInstructionto enforce debate rules at the system level.This ensures debate constraints always take precedence over persona styling.
2. Prompt Construction Refactor
Files:
debatevsbot.goconstructPromptto return two distinct prompts:This aligns prompt structure with Gemini’s intended system-instruction design and improves response consistency.
3. Output Validation and Retry Logic
Files:
debatevsbot.govalidateResponseto detect:This prevents low-quality responses from reaching the UI.
4. Signature Alignment Across Services
Files:
transcriptservice.gopros_cons.gocoach.goUpdated all AI generation calls to match the revised Gemini function signatures.
Ensured no regressions across other AI-powered features.
Verification
Tests
TestConstructPrompt(services package) to verify:Result: ✅ Pass
Build
Result: ✅ Success
Outcome
Scope & Impact
Checklist
closes #250
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.