Skip to content

Merge main to dev#12

Merged
rxolve merged 5 commits intodevfrom
main
Mar 2, 2026
Merged

Merge main to dev#12
rxolve merged 5 commits intodevfrom
main

Conversation

@rxolve
Copy link
Copy Markdown
Collaborator

@rxolve rxolve commented Mar 2, 2026

No description provided.

rxolve and others added 5 commits March 2, 2026 08:32
feat: dialectic debate narrative in PR review output
fix: remove process.env credential pollution re-introduced by merge
refactor: centralize model, file classification, and strategy constants
- Update model references from claude-sonnet-4-20250514 to claude-opus-4-6
- Update workflow example from @v1 to @main
- Add review output format section with 3-step debate example
- Update Hawk/Owl section to describe STEP 1/2/3 structure
- Rewrite CHANGELOG with [Unreleased] section and accurate 1.0.0 stats
- Remove outdated Extended Thinking references
- Fix test count (104), module count (30), line count (~5,800)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Set language default to "en" (was undefined)
- Make language field non-optional in DialecticConfig
- Fix JSON parser to detect unfenced consensus JSON blocks
  When Claude omits code fences, the parser now finds JSON by
  matching {"consensus_completed": ...} pattern as fallback,
  fixing FP Rate showing "—" and verdict defaulting to LGTM

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rxolve rxolve self-assigned this Mar 2, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 2, 2026

🤖 Dialectic PR Review

Framework: vanilla
Strategy: small
Files Reviewed: 5

Summary

The language default change (language?: stringlanguage: string = "en") is a clean improvement that eliminates undefined propagation. The CHANGELOG and README updates are documentation-only and look correct. The main concern is the new unfenced JSON regex fallback in consensus-engine.ts: the regex will fail on any JSON with nested objects containing \n}, which is virtually guaranteed for real consensus output. This is a high-confidence parsing regression that should be addressed before merge.

Issues

🐛 Greedy regex with non-greedy inner quantifier may truncate or mismatch JSON

File: src/core/consensus-engine.ts (Line 415)
Type: bug
Confidence: high

The regex /\{\s*"consensus_completed"\s*:\s*[\s\S]*?\n\}/ uses a non-greedy [\s\S]*? followed by \n\}. This will match the first occurrence of \n} after the opening brace, which will almost certainly truncate a nested JSON object. For example, if the JSON contains nested objects like { "consensus_completed": true, "agreed_issues": [ { "id": 1 } ] }, the regex will stop at the first \n} inside agreed_issues, producing invalid JSON. The previous fallback (entire response as JSON) was actually safer for well-formed responses.

Suggestion:

Instead of regex-matching nested JSON, use a brace-counting parser or find the outermost { starting with "consensus_completed" and count balanced braces to extract the full JSON object. Alternatively, use JSON.parse with progressive substring trimming from the end of the response.


🐛 Non-null assertion on unfencedMatch.index without guard

File: src/core/consensus-engine.ts (Line 417)
Type: bug
Confidence: medium

unfencedMatch.index! uses a non-null assertion. While RegExp.prototype.match does populate index when a match is found, this is a TypeScript type-level concern — index is typed as number | undefined on RegExpMatchArray. If the runtime behavior ever changes or the code is refactored to use a different matching method (e.g., matchAll), this could silently become undefined, causing substring(0, undefined) to return the entire string rather than the narrative prefix.

Suggestion:

Add an explicit guard: const matchStart = unfencedMatch.index ?? 0; or check if (unfencedMatch.index !== undefined) before using it.


Metadata

  • Tokens Used: 8,611
  • Duration: 16.31s

Powered by Dialectic PR Review

@rxolve rxolve merged commit c893fca into dev Mar 2, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant