Skip to content

test(agent): use hosted inference import alias - #4410

Closed
kojiwakayama wants to merge 1 commit into
mainfrom
test/hosted-inference-import-alias
Closed

kojiwakayama wants to merge 1 commit into
mainfrom
test/hosted-inference-import-alias

Conversation

@kojiwakayama

Copy link
Copy Markdown
Contributor

Summary

Verification

  • hosted chat parser suite: 89 steps passed
  • deno fmt --check and deno lint
  • dependency and module boundary checks
  • test typecheck baseline
  • codex review --base origin/main: no actionable findings

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

kojiwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 21 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 81f2a485-9cb1-4947-88cb-3bf4cd75ca39

📥 Commits

Reviewing files that changed from the base of the PR and between 94350f0 and 5ae1c23.

📒 Files selected for processing (1)
  • src/agent/hosted/chat-request.test.ts

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.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 288 2279 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 5ae1c23071

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link
Copy Markdown
Contributor

Review: 35/100 — significant concerns (change is directionally wrong)

This is a 1-line, single-file diff, but the direction of the change is the opposite of what this repo's own conventions call for, so it shouldn't merge as-is.

Findings:

  • The import change contradicts the project's documented and enforced convention. .claude/skills/vf-module-development/SKILL.md states: "Within the same module, use relative imports." inference-credential.ts and chat-request.test.ts are both in src/agent/hosted/ — same directory, same module. The pre-PR ./inference-credential.ts was already correct.
  • The repo's actual lint rule agrees, and explicitly exempts this file twice over. scripts/lint/no-cross-boundary-relative-imports.ts only flags ../-style imports that cross a top-level module boundary (src/agent/hosted/renderer.ts → ../../cache/keys.ts), and it explicitly skips .test.ts files altogether (shouldSkip). ./inference-credential.ts is neither a ../ import nor cross-module, and the file is a test file — this line was never going to trip that check.
  • This is inconsistent with 29 other test files in the same directory (46 occurrences) that use same-directory relative imports (./foo.ts) for their sibling source modules — e.g. chat-preparation.test.ts, veryfront-cloud-agent-service.test.ts, child-lifecycle.test.ts. Merging this makes chat-request.test.ts the outlier, not the model to follow.
  • Root cause: this PR blindly applies a CodeRabbit "outside-diff" suggestion from fix(agent): bind inference token on default chat dispatch #4407 whose stated rationale ("do not add relative internal imports outside cli/") is not this repo's actual guideline — it appears to be a stale/generic rule from CodeRabbit's own learnings, not sourced from this repo's skill docs or lint script. Worth double-checking/correcting that CodeRabbit learning so it stops generating this suggestion on future PRs.
  • No test coverage or behavior change involved (it's an import path only), and #veryfront/agent/hosted/inference-credential.ts does resolve correctly via the "#veryfront/": "./src/" catch-all in deno.json, so this won't break the build — the issue is purely convention/consistency, not correctness.

Recommendation: close this PR (or revert to ./inference-credential.ts) rather than merging. If the CodeRabbit finding on #4407 is going to keep recurring, it's worth telling CodeRabbit (via a learning/comment) that same-directory relative imports are intentional in this repo, rather than "fixing" each instance it flags.


Generated by Claude Code

@gitar-bot

gitar-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review ✅ Approved

Updates hosted inference credential test import to use the canonical internal alias, addressing a CodeRabbit finding. All verification checks passed with no issues found.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Important

Your trial ends in 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sonarqubecloud

sonarqubecloud Bot commented Sep 4, 2026

Copy link
Copy Markdown

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Thanks — this is actionable and correct. The source and test are in the same module, where this repository intentionally uses same-directory relative imports; the boundary lint also does not prohibit this case. I am closing this follow-up rather than merging a convention regression. The original outside-diff suggestion on #4407 should be treated as inapplicable.

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