Skip to content

test(gjc): read the started process until herdr shows it with its sessions directory - #1807

Merged
devswha merged 1 commit into
mainfrom
fix/gjc-start-process-read
Oct 24, 2026
Merged

devswha merged 1 commit into
mainfrom
fix/gjc-start-process-read

Conversation

@devswha

@devswha devswha commented Oct 24, 2026

Copy link
Copy Markdown
Owner

Fixes #1462.

Summary

  • server/gjc.contract.test.ts, starts the GJC executable found on the server PATH by absolute path: the test read pane.process_info once, right after startShellAgent returned, and failed once on main (CI run 39440414420) when that read found no gjc process. It now reads through the file's existing until helper, with a 10 s deadline, until a foreground process matches isGjcProcess with the sessions directory as its last argument, as the issue asks. The assertion on that argument stays.
  • Test-only: no product code changes, so no changelog fragment.

Verification

  • The test 5/5 runs, server/gjc.contract.test.ts 10/10, typecheck, bun run check fast.
  • Not reproduced on demand: the failure depends on herdr's process_info timing right after the start, and none of these runs hit it.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between c78fda4 and 8df73bf.

Files selected for processing: 1
  • server/gjc.contract.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Tests
    • Improved the reliability of automated checks for PATH-based startup. The check now waits for process details to become available before verifying the startup command, reducing failures caused by timing differences during launch. This update changes test behavior only and does not alter the application’s functionality.

The GJC PATH startup test now polls pane.process_info for up to 10 seconds until the foreground GJC process has the sessions directory as its final argument. The test then checks that argument.

Changes: GJC startup test

Layer / File(s) Summary
Wait for the expected GJC process
server/gjc.contract.test.ts
The test polls pane.process_info until the foreground GJC process has the sessions directory as its final argument, then checks that argument.

Priority: Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Other · Severity of issue fixed: Low

Possibly related PRs

  • devswha/ovis#1542: Adds the absolute-path GJC startup coverage that this test now polls.

Suggested reviewers: kinetic27

Merge Risk: Minimal · up to 8df73

The test now uses bounded polling for the expected GJC process. No actionable merge risk is established; normal checks remain appropriate.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly describes the main test change: polling the started GJC process until its sessions directory appears.
Description check Passed The description explains the failure, resulting behavior, issue reference, test-only scope, validation results, and lack of a changelog fragment. The headings differ from the template, but the require…
Linked Issues check Passed Issue #1462 requires bounded polling of pane.process_info after startShellAgent, without a fixed sleep. The diff adds until(Date.now() + 10_000, ...) and stops when a foreground process passes `…
Out of Scope Changes check Passed The whole-PR diff changes only server/gjc.contract.test.ts. The change strengthens the test for the startup race described by issue #1462 and adds no unrelated product or repository changes.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Full details: Description check

Explanation

The description explains the failure, resulting behavior, issue reference, test-only scope, validation results, and lack of a changelog fragment. The headings differ from the template, but the required information is present.

Full details: Linked Issues check

Explanation

Issue #1462 requires bounded polling of pane.process_info after startShellAgent, without a fixed sleep. The diff adds until(Date.now() + 10_000, ...) and stops when a foreground process passes isGjcProcess and its last argument equals sessions. The final assertion remains.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@devswha

devswha commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Merge pass. A gate reviewer on a different model than the author's reviewed this and found no blocking issues.

The new wait: it polls the real foreground-process state for up to 10 s. Its 50 ms interval passes the bounded-poll conventions rule. A missing GJC, or a wrong argument that never changes, still fails, and the original assertion stays.

Runs:

  • The changed test passed 3/3 isolated runs.
  • server/conventions and the typecheck pass.
  • The full file passed 10/10, 8/10 and 10/10. In the 8/10 run, two tests this PR doesn't change failed before the changed test ran:
    • gjc.contract.test.ts:104: pane.process_info did not expose the isolated GJC process, the same single-read race this PR fixes for one test;
    • ConversationUnavailable: no_session_path (server/conversation.ts:683).

Follow-up: give those reads the same until wait. #1462 is fixed for this test only.

All required checks are green and no review thread is open.

@devswha
devswha merged commit c0d70e5 into main Oct 24, 2026
17 checks passed
@devswha
devswha deleted the fix/gjc-start-process-read branch October 24, 2026 04:53
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.

Flaky: gjc contract test reads process_info once after startShellAgent

1 participant