fix(agents-cli) Inkeep pull uses human-readable stems for file imports#2468
fix(agents-cli) Inkeep pull uses human-readable stems for file imports#2468miles-kt-inkeep wants to merge 6 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 92f36e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Summary: Makes
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is clean — two-pass collect-then-generate so all file stems are known before any generator emits import paths. Two issues to address:
Bug: context config fallback paths not updated. collectContextConfigRecords (line 360) and collectAgentContextTemplateReferences (line 995) in introspect-generator.ts still use `${contextConfigId}.ts` as the fallback, while every other collector uses buildComponentFileName. On a fresh pull with opaque IDs, this produces e.g. cm82v49jh001upc01lo2ddxn2.ts instead of my-context-config-lo2ddxn2.ts, and the import stem in _importStems will be the raw UUID — mismatching the file name other generators expect.
registerAllComponents is dead code. It is exported from component-registry.ts but never imported or called anywhere. The changes to it in this PR have no runtime effect. The actual file path assignments come from the individual collect*Records functions in introspect-generator.ts. Not blocking, but worth knowing — the function could be removed or wired up in a follow-up.
| let _importStems: Record<string, string> = {}; | ||
|
|
||
| export function setImportStems(stems: Record<string, string>): void { | ||
| _importStems = stems; | ||
| } | ||
|
|
||
| export function getImportStem(id: string): string { | ||
| return _importStems[id] ?? id; | ||
| } |
There was a problem hiding this comment.
Module-level mutable state with no reset. If introspectGenerate is called more than once in the same process (tests, watch mode, or a future batch workflow), stale stems from a previous run persist and silently win via the ??= in the collection loop. Consider calling _importStems = {} at the top of introspectGenerate (or adding a clearImportStems() helper) so each invocation starts clean.
| collectedRecords.push({ generate: task.generate, record }); | ||
| importStems[record.id] ??= basename(record.filePath, '.ts'); |
There was a problem hiding this comment.
The stem for context configs will be wrong on a fresh pull. collectContextConfigRecords (line 360) still uses `${contextConfigId}.ts` as the fallback file path, so basename(record.filePath, '.ts') here produces the raw UUID. Every other collector passes the name through buildComponentFileName. The same issue exists at line 995 (collectAgentContextTemplateReferences).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
utils.ts:165Silent fallback ingetImportStem()produces broken imports when component ID is missing - 🟠 Major:
utils.ts:160-166NewsetImportStems()/getImportStem()functions lack unit test coverage
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
utils.ts:158-166Module-level state lacksclearImportStems()reset function for test isolation
🕐 Pending Recommendations (3)
- 🔴
introspect-generator.ts:360Context config fallback path still uses${contextConfigId}.tsinstead ofbuildComponentFileName() - 🔴
introspect-generator.ts:995Same issue incollectContextTemplateReferences()fallback path - 💭
component-registry.ts:517-665registerAllComponents()is dead code — changes have no runtime effect
🚫 REQUEST CHANGES
Summary: The two-pass collect-then-generate architecture is clean and correct for solving the human-readable file naming problem. However, there are two critical gaps that need addressing before this PR can ship reliably:
-
Context config fallback paths at lines 360 and 995 still use raw IDs instead of
buildComponentFileName(). This means context configs with opaque CUIDs will get incorrectly named files, and the import stems will mismatch. This was already identified in the prior review and remains unresolved. -
The silent fallback in
getImportStem()masks bugs by producing broken imports that only fail at TypeScript compile time. Consider failing fast with a descriptive error to surface registration issues duringinkeep pullrather than deferring failures.
The test coverage and reset function suggestions are important for maintainability but are less urgent than fixing the context config paths.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
utils.ts:158 |
Naming convention inconsistency — underscore prefix _importStems |
Mixed precedent in codebase; 50/50 developer preference |
introspect-generator.ts:150 |
??= operator silently ignores duplicate IDs |
Intentional first-wins semantics; edge case unlikely in practice |
component-registry.ts |
Dead code changes | Already raised in prior review — routed to Pending |
| Generator tests | Tests don't use opaque IDs | Useful improvement but not blocking; existing integration tests exercise the paths |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-errors |
3 | 0 | 0 | 0 | 2 | 0 | 1 |
pr-review-tests |
4 | 0 | 0 | 0 | 1 | 2 | 1 |
pr-review-consistency |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
pr-review-precision |
3 | 0 | 0 | 0 | 0 | 3 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 12 | 0 | 0 | 0 | 3 | 3 | 4 |
Note: Standards reviewer found no issues beyond what was already raised in prior review.
| } | ||
|
|
||
| export function getImportStem(id: string): string { | ||
| return _importStems[id] ?? id; |
There was a problem hiding this comment.
🟠 MAJOR: Silent fallback produces broken imports
Issue: getImportStem(id) silently falls back to the raw ID when a component is not found in _importStems. This produces syntactically valid but semantically broken import paths that fail at TypeScript compile time with "Cannot find module" errors.
Why: If a component ID isn't registered (due to a missing collector, typo, or unsupported component type), generated imports will reference non-existent files like ../credentials/cm82v49jh001upc01lo2ddxn2 instead of ../credentials/api-key-lo2ddxn2. This failure only surfaces after full code generation, making debugging difficult since the root cause (missing stem) isn't obvious from the TypeScript error.
Fix: Consider failing fast with a descriptive error:
export function getImportStem(id: string): string {
const stem = _importStems[id];
if (!stem) {
throw new Error(
`Import stem not found for component ID '${id}'. ` +
`Ensure the component is registered via collectXxxRecords() before code generation.`
);
}
return stem;
}Alternatively, if graceful degradation is acceptable, emit a warning:
if (!stem) {
console.warn(`Warning: Import stem not found for '${id}'. Using raw ID as fallback.`);
return id;
}Refs:
| let _importStems: Record<string, string> = {}; | ||
|
|
||
| export function setImportStems(stems: Record<string, string>): void { | ||
| _importStems = stems; | ||
| } | ||
|
|
||
| export function getImportStem(id: string): string { | ||
| return _importStems[id] ?? id; | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Missing reset function for test isolation
Issue: The _importStems module-level state has a setter but no reset function. While setImportStems() replaces the entire object (avoiding stale entry accumulation), there's no explicit clearImportStems() for test teardown.
Why: The codebase convention for module-level state (e.g., resetDevConfigCache, resetSpiceClient, resetEnv, clearJwksCache) includes reset/clear functions to ensure test isolation. Generator unit tests that call getImportStem() currently rely on the fallback behavior without verifying stem resolution works correctly with populated data.
Fix: Consider adding a reset helper:
| let _importStems: Record<string, string> = {}; | |
| export function setImportStems(stems: Record<string, string>): void { | |
| _importStems = stems; | |
| } | |
| export function getImportStem(id: string): string { | |
| return _importStems[id] ?? id; | |
| } | |
| let _importStems: Record<string, string> = {}; | |
| export function setImportStems(stems: Record<string, string>): void { | |
| _importStems = stems; | |
| } | |
| export function clearImportStems(): void { | |
| _importStems = {}; | |
| } | |
| export function getImportStem(id: string): string { | |
| return _importStems[id] ?? id; | |
| } |
Refs:
| export function setImportStems(stems: Record<string, string>): void { | ||
| _importStems = stems; | ||
| } | ||
|
|
||
| export function getImportStem(id: string): string { | ||
| return _importStems[id] ?? id; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: New functions lack unit test coverage
Issue: The setImportStems() / getImportStem() functions implement the core import path resolution logic but have no dedicated unit tests. These functions are called by every generator but are only exercised indirectly through integration tests that use human-readable fixture IDs.
Why: Without tests, the following bugs could be introduced undetected: (1) If getImportStem is called before setImportStems, all imports use raw IDs, (2) The fallback behavior (_importStems[id] ?? id) is untested, (3) There's no verification that module-level state is correctly initialized.
Fix: Add unit tests in utils.test.ts:
describe('getImportStem', () => {
afterEach(() => {
setImportStems({}); // Reset state
});
it('returns id when no stems are set', () => {
expect(getImportStem('cm82v49jh001upc01lo2ddxn2')).toBe('cm82v49jh001upc01lo2ddxn2');
});
it('returns mapped stem when set', () => {
setImportStems({ 'cm82v49jh001upc01lo2ddxn2': 'support-agent-lo2ddxn2' });
expect(getImportStem('cm82v49jh001upc01lo2ddxn2')).toBe('support-agent-lo2ddxn2');
});
it('returns id for unmapped keys', () => {
setImportStems({ 'other-id': 'other-stem' });
expect(getImportStem('missing-id')).toBe('missing-id');
});
});Refs:
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (3) Pending | Risk: Medium
Delta Review: c468a6cdf — setImportStems({}) reset
The single-line change at introspect-generator.ts:136 correctly addresses the prior feedback about module-level state persistence. By calling setImportStems({}) at the start of introspectGenerate(), stale stems from previous runs are cleared — preventing silent bugs in watch mode, tests, or batch workflows.
No new issues introduced by this delta.
🕐 Pending Recommendations (3)
These issues were raised in prior reviews and remain unresolved:
- 🔴
introspect-generator.ts:361Context config fallback path uses${contextConfigId}.tsinstead ofbuildComponentFileName() - 🔴
introspect-generator.ts:996Same issue incollectAgentContextTemplateReferences()fallback path - 🟠
utils.ts:165Silent fallback ingetImportStem()produces broken imports when component ID is missing
Notes
- The
clearImportStems()export suggested in prior feedback was not added, but the current approach (setImportStems({})) achieves the same effect and is already in use. Acceptable. - The test coverage gap for
setImportStems()/getImportStem()remains but is not blocking. registerAllComponents()dead code remains — cosmetic, not blocking.
🚫 REQUEST CHANGES
Summary: The delta commit is correct and addresses the module-level state reset concern. However, the context config fallback path issue (lines 361 and 996) raised in prior reviews remains unresolved. This is a bug that will cause context configs with opaque CUIDs to generate incorrectly named files and broken import paths. Please address before merging.
Discarded (0)
No new findings to discard in this delta review.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| orchestrator-only | — | 0 | 0 | 0 | 0 | 3 | 0 |
Note: Delta review — no subagent reviewers dispatched for a 1-line change that was already analyzed in prior reviews.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (0) Pending | Risk: Low
Delta Review: 844541000 — "feedback"
The latest commit addresses all three critical issues raised in prior reviews:
1. Context config fallback paths now use buildComponentFileName()
collectContextConfigRecords()at line ~365 — ✅ FixedcollectContextTemplateReferences()at line ~1005 — ✅ Fixed
Both locations now extract contextConfigName and pass it through buildComponentFileName(contextConfigId, contextConfigName), ensuring context configs with opaque CUIDs generate human-readable file names like my-context-config-lo2ddxn2.ts instead of raw UUIDs.
2. getImportStem() now warns on missing stems
- Added
_importStemsActiveflag (line 159) that tracks whether stems have been populated - When
getImportStem()is called with a non-human-readable ID that isn't registered, it now emits aconsole.warn()(lines 171-174) - This provides early feedback during
inkeep pullrather than deferring failures to TypeScript compile time
Implementation quality:
- The warning only fires when stems are active AND the ID appears to be a CUID/UUID (via
isHumanReadableId(id)check), avoiding spurious warnings for intentionally human-readable IDs - The fallback to raw ID is preserved for backward compatibility with existing workflows
No new issues introduced by this delta.
Prior Feedback Status
| Prior Issue | Status |
|---|---|
| Context config fallback at line ~360 | ✅ Resolved in this commit |
| Context config fallback at line ~995 | ✅ Resolved in this commit |
Silent fallback in getImportStem() |
✅ Resolved — now warns |
| Module-level state reset | ✅ Already resolved in prior commit (setImportStems({}) at line 136) |
Test coverage for setImportStems()/getImportStem() |
💭 Not blocking — existing integration tests exercise the paths |
registerAllComponents() dead code |
💭 Cosmetic — not blocking, could be removed in follow-up |
✅ APPROVE
Summary: All critical issues from prior reviews have been addressed. The two-pass collect-then-generate architecture is clean, the context config paths now correctly use buildComponentFileName(), and getImportStem() provides helpful warnings for missing stems. The implementation is ready to ship. 🚀
Discarded (0)
No findings to discard — delta is clean.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| orchestrator-only | — | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review — all prior critical issues were addressed. No subagent reviewers dispatched for a focused 2-file delta that directly resolves previously-identified bugs.
Ito Test Report ✅18 test cases ran. 18 passed. This test run verified the new human-readable import stem functionality for the ✅ Passed (18)
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (0) Pending | Risk: Low
Delta Review: f13bab72d — Merge from main
This is a delta review scoped to changes since the last automated review (commit 844541000).
Delta contents:
- Merge commit from
mainbranch - Unrelated PRs merged to main: playground conversation reset fix (#2473), SigNoz error logging (#2469), version packages (#2453)
No changes to the actual PR files (agents-cli/src/commands/pull-v4/*) since the last review.
Prior Review Status
The previous review at commit 844541000 gave an ✅ APPROVE with all critical issues addressed:
| Prior Issue | Status |
|---|---|
| Context config fallback at line ~360 | ✅ Resolved |
| Context config fallback at line ~995 | ✅ Resolved |
Silent fallback in getImportStem() |
✅ Resolved — now warns |
| Module-level state reset | ✅ Resolved |
| Test coverage for new functions | 💭 Not blocking |
registerAllComponents() dead code |
💭 Cosmetic — follow-up |
No new issues introduced by this delta.
✅ APPROVE
Summary: The delta contains only a merge from main with unrelated changes — no modifications to the PR's actual implementation. The prior review's approval stands. The two-pass collect-then-generate architecture is clean, context config paths correctly use buildComponentFileName(), and getImportStem() provides helpful warnings for missing stems. Ready to ship! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| orchestrator-only | — | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review with no PR file changes — no subagent reviewers dispatched.
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | (0) Pending | Risk: Low
Delta Review: 92f36e703 — Merge from main
This is a delta review scoped to changes since the last automated review (commit f13bab72d).
Delta contents:
- Merge commit from
mainbranch - Unrelated PRs merged to main: copilot token fix (#2491), docker-compose updates (#2490), scheduled trigger timezone (#2487), email integration (#2038), stepper component (#2480), API key restriction (#2478), version packages
No changes to the actual PR files (agents-cli/src/commands/pull-v4/*) since the last review.
Prior Review Status
The previous review at commit f13bab72d gave an ✅ APPROVE with all critical issues addressed:
| Prior Issue | Status |
|---|---|
| Context config fallback at line ~360 | ✅ Resolved |
| Context config fallback at line ~995 | ✅ Resolved |
Silent fallback in getImportStem() |
✅ Resolved — now warns |
| Module-level state reset | ✅ Resolved |
| Test coverage for new functions | 💭 Not blocking |
registerAllComponents() dead code |
💭 Cosmetic — follow-up |
No new issues introduced by this delta.
✅ APPROVE
Summary: The delta contains only a merge from main with unrelated changes — no modifications to the PR's actual implementation. The prior review's approval stands. The two-pass collect-then-generate architecture is clean, context config paths correctly use buildComponentFileName(), and getImportStem() provides helpful warnings for missing stems. Ready to ship! 🎉
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| orchestrator-only | — | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta review with no PR file changes — no subagent reviewers dispatched.
1 participant
No description provided.