Skip to content

Conversation

@dsarno
Copy link
Owner

@dsarno dsarno commented Sep 6, 2025

Summary

  • make Unity bridge wait step tolerant of startup licensing chatter
  • detect readiness via status file port or log markers with warm-up window
  • canonicalize NL/T test fragment names and backfill missing tests to avoid "not run" markers
  • harden NL/T prompt with a strict result-emission rule
  • fix workflow YAML syntax so GitHub registers the job

Testing

  • ruby -ryaml -e 'puts YAML.load_file(".github/workflows/claude-nl-suite.yml").keys'
  • pytest

https://chatgpt.com/codex/tasks/task_e_68bb8d4b31048327857b63789cb4cd2b

@coderabbitai
Copy link

coderabbitai bot commented Sep 6, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/fix-wait-gate-for-unity-licensing-readiness-flepwk

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR significantly improves the Unity bridge wait-gate resilience by addressing reliability issues with Unity's licensing startup behavior. The changes replace a brittle timeout-based approach with a more sophisticated dual-detection system that can handle Unity's transient licensing chatter during startup.

The core improvement is in the GitHub Actions workflow where the wait-gate logic is completely rewritten to use deadline-based timing instead of nested bash timeouts. The new approach implements two readiness detection mechanisms: primary detection via status JSON file port verification with TCP connection testing, and secondary detection via log pattern matching for specific readiness markers. Crucially, it introduces a 2-minute warm-up window during which licensing errors are tolerated, only treating them as fatal after this period.

Additionally, the PR addresses test reporting consistency by adding canonicalization logic that standardizes NL/T test fragment names using regex patterns, and implements backfill functionality that creates failure placeholders for missing tests to prevent "not run" markers in CI reports. The NL/T test prompt is hardened with strict result-emission rules that enforce standardized XML output format with exact test ID naming (NL-0 through NL-4 and T-A through T-J).

These changes integrate with the existing Unity MCP bridge architecture by making the CI workflow more robust while maintaining compatibility with the existing test framework and reporting structure. The workflow syntax fixes ensure proper GitHub job registration for the enhanced testing pipeline.

Important Files Changed

Changed Files
Filename Score Overview
.github/workflows/claude-nl-suite.yml 4/5 Completely rewrites Unity bridge wait-gate logic with dual readiness detection, licensing tolerance, and test canonicalization/backfill functionality
.claude/prompts/nl-unity-suite-full-additive.md 5/5 Adds strict result emission rules to standardize XML test output format and enforce canonical test ID naming

Confidence score: 4/5

  • This PR is generally safe to merge with well-thought-out improvements to CI reliability
  • Score reflects solid architectural improvements but complexity in the wait-gate logic warrants careful verification
  • Pay close attention to the GitHub Actions workflow file due to the significant rewrite of timing and detection logic

2 files reviewed, 1 comment

Edit Code Review Bot Settings | Greptile

Comment on lines +427 to +429
if re.search(pat, n, flags=re.I):
suffix = re.sub(rf"^\s*{re.escape(tid)}\s*[:.\-–—]?\s*", "", n, flags=re.I)
return f"{tid}" + (f": {suffix}" if suffix.strip() else "")
Copy link

Choose a reason for hiding this comment

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

style: The regex substitution removes the test ID prefix from the name then re-adds it - this could potentially create edge cases where the pattern matches incorrectly and produces malformed names.

@dsarno dsarno merged commit 0d43cfd into nl-CI-workflow-fixes Sep 6, 2025
1 check passed
@dsarno dsarno deleted the codex/fix-wait-gate-for-unity-licensing-readiness-flepwk branch September 6, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants