Skip to content

testkit: OUTPUT_PATTERN guard; prebuilt CLI for spec-kit CI#7

Merged
bschellenberger2600 merged 2 commits into
mainfrom
cursor/low-priority-testkit-polish-c137
Apr 16, 2026
Merged

testkit: OUTPUT_PATTERN guard; prebuilt CLI for spec-kit CI#7
bschellenberger2600 merged 2 commits into
mainfrom
cursor/low-priority-testkit-polish-c137

Conversation

@bschellenberger2600
Copy link
Copy Markdown
Member

@bschellenberger2600 bschellenberger2600 commented Apr 8, 2026

Summary

  • CliBridge: runGitCmd uses extractRequired(json, OUTPUT_PATTERN, "output") only, consistent with getCurrentSha / getBranches (no redundant pre-check; per CodeRabbit review).
  • CI (spec-kit-conformance): After go build ./cmd/git-testkit-cli, Python and Maven steps set GIT_TESTKIT_CLI=./git-testkit-cli so wrappers invoke the prebuilt binary instead of go run on every subprocess.
  • Docs: testkit/README.md describes the Go regression root as the directory containing go.mod (avoids implying a fixed path like /workspace). testkit/ROADMAP.md spells out that the CLI lives under cmd/git-testkit-cli, not a separate testkit/cli tree.

Notes

On current main, invoke() already uses OK_PATTERN for the ok field; the wrapper-cross-platform job already builds the CLI and sets GIT_TESTKIT_CLI. There is no GitTestKitCLI alias in cli.py anymore.

Testing

  • go test -short ./... (Java/Maven not available in this environment).
Open in Web Open in Cursor 

Summary by CodeRabbit

  • Documentation

    • Clarified command execution instructions for repository setup.
    • Updated project roadmap specifications.
  • Chores

    • Enhanced continuous integration workflow configuration.
  • Refactor

    • Improved validation logic for better reliability.

- CliBridge.runGitCmd: detect missing output via OUTPUT_PATTERN instead of a
  fragile substring match on quoted field names.
- spec-kit-conformance job: set GIT_TESTKIT_CLI to the binary built in the
  prior step so Python/Java steps avoid repeated go run recompiles.
- Clarify testkit README Go regression path and ROADMAP CLI location wording.

Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 8, 2026

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'testkit: OUTPUT_PATTERN guard; prebuilt CLI for spec-kit CI' accurately summarizes the main changes: updating CliBridge to use OUTPUT_PATTERN for validation and configuring CI to use a prebuilt CLI binary instead of go run.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cursor/low-priority-testkit-polish-c137

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.

@bschellenberger2600 bschellenberger2600 marked this pull request as ready for review April 8, 2026 15:16
@cursor
Copy link
Copy Markdown

cursor Bot commented Apr 8, 2026

You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace.

To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java (1)

257-260: Good improvement to use regex-based validation.

Using OUTPUT_PATTERN.matcher(json).find() instead of a substring check is more robust and consistent with how other fields are validated.

Optional: The pre-check on line 257 is redundant since extractRequired on line 260 already throws if the pattern doesn't match. Other methods like getCurrentSha and getBranches call extractRequired directly without pre-checks. Consider removing lines 257-259 if maintaining consistency with those methods is preferred.

,

♻️ Optional simplification
    payload.append("]}");
    String json = invoke(payload.toString());
-   if (!OUTPUT_PATTERN.matcher(json).find()) {
-     throw new IllegalStateException("missing output in bridge response: " + json);
-   }
    return extractRequired(json, OUTPUT_PATTERN, "output");
  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java` around lines
257 - 260, Remove the redundant pre-check that calls
OUTPUT_PATTERN.matcher(json).find() before returning extractRequired(json,
OUTPUT_PATTERN, "output"); instead rely on extractRequired to perform the match
and throw if missing (consistent with getCurrentSha/getBranches). Update the
method in CliBridge to delete the if-block referencing
OUTPUT_PATTERN.matcher(json).find() and simply return extractRequired(json,
OUTPUT_PATTERN, "output").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java`:
- Around line 257-260: Remove the redundant pre-check that calls
OUTPUT_PATTERN.matcher(json).find() before returning extractRequired(json,
OUTPUT_PATTERN, "output"); instead rely on extractRequired to perform the match
and throw if missing (consistent with getCurrentSha/getBranches). Update the
method in CliBridge to delete the if-block referencing
OUTPUT_PATTERN.matcher(json).find() and simply return extractRequired(json,
OUTPUT_PATTERN, "output").

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9be183be-d7a4-494c-b057-01dc0aeecffa

📥 Commits

Reviewing files that changed from the base of the PR and between ad81ae5 and 175f0a5.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • testkit/README.md
  • testkit/ROADMAP.md
  • testkit/java/src/main/java/io/gitfire/testkit/CliBridge.java

@bschellenberger2600
Copy link
Copy Markdown
Member Author

@cursoragent address feedback then kick off bugbot review

@cursor
Copy link
Copy Markdown

cursor Bot commented Apr 16, 2026

Here is what was done and what is blocked on your side.

Code review feedback (CodeRabbit)

The redundant OUTPUT_PATTERN.matcher(json).find() block before extractRequired in runGitCmd was removed so behavior matches getCurrentSha / getBranches: only extractRequired(json, OUTPUT_PATTERN, "output") runs.

That change is committed and pushed on cursor/low-priority-testkit-polish-c137 (commit 6dcfac6). The PR description was updated to match (no more claim about a separate OUTPUT pre-guard).

Bugbot review

Posting a PR comment with gh pr comment failed with:

GraphQL: Resource not accessible by integration (addComment)

So this environment’s GitHub token cannot add comments, and I cannot trigger Bugbot from here by commenting cursor review / bugbot run.

What you should do: On PR #7, add a comment as yourself (or another user the integration allows), for example:

cursor review

or

bugbot run

That is what Cursor’s docs describe for a manual Bugbot run. If you still see the trial limit message from @cursor[bot], you’ll need Bugbot enabled for the org/account in the Cursor dashboard as that message indicated.

Tests

go test -short ./... failed once in this VM on an unrelated git commit signing / agent error, not on the Java change. Re-run locally or in CI when signing is configured normally.

Open in Web Open in Cursor 

extractRequired already fails with a clear message when output is missing;
matches getCurrentSha/getBranches style (CodeRabbit review).

Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
@bschellenberger2600 bschellenberger2600 merged commit 0993b45 into main Apr 16, 2026
8 checks 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.

2 participants