inkeep pull, split introspect-generator.ts in individual generator task.collect functions#2654
inkeep pull, split introspect-generator.ts in individual generator task.collect functions#2654dimaMachina wants to merge 5 commits intomainfrom
inkeep pull, split introspect-generator.ts in individual generator task.collect functions#2654Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude --review |
|
TL;DR — Decomposes the monolithic Key changesSummary ｜ 52 files ｜ 1 commit ｜ base:
|
| Module | Responsibility |
|---|---|
code-values.ts |
CodeValue union type, codeReference(), codeExpression(), codePropertyAccess(), formatInlineLiteral() |
factory-writer.ts |
createFactoryDefinition(), addFactoryConfigVariable(), addValueToObject() |
naming.ts |
toCamelCase(), toKebabCase(), buildComponentFileName(), naming collision helpers |
schema-rendering.ts |
JSON Schema → Zod string conversion |
shared.ts |
isPlainObject(), isHumanReadableId() |
templates.ts |
formatTemplate(), formatStringLiteral(), formatPropertyName() |
utils/index.ts · utils/code-values.ts · utils/factory-writer.ts · utils/naming.ts
reference-resolution.ts — import binding deduplication
Before: Each generator manually resolved import aliases and handled name collisions inline with different strategies.
After:resolveReferenceBindings()accepts a list ofReferenceResolutionInputentries and producesResolvedReferenceBindingobjects with pre-resolvedlocalName,namedImport(with optional alias), andmodulePath. Supports three collision strategies:descriptive(appends a suffix),numeric(appends index), andnumeric-for-duplicates(numeric only when the sameimportNameappears multiple times).
docs/pull.md — internal architecture documentation
Before: No written documentation for the
pull-v4pipeline.
After: 263-line document covering entry points,inkeep pulllifecycle, target directory resolution, remote normalization, generated layout, generator pipeline order, merge behavior, file-scope merging, and the module structure introduced in this PR.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured refactoring that breaks a ~2700-line monolith into focused, cohesive modules. The GenerationResolver class cleanly centralizes reference resolution, and the import.meta.glob auto-discovery is a nice pattern for keeping the task registry maintenance-free. Three items to address: the documentation lists an explicit task order that no longer matches reality, asRecord/stripExtension are duplicated across three files, and there's a helper function duplication between collector-common.ts and generators/helpers/agent.ts.
| The current task order is: | ||
|
|
||
| 1. credentials | ||
| 2. environment settings | ||
| 3. environment index | ||
| 4. artifact components | ||
| 5. data components | ||
| 6. function tools | ||
| 7. MCP tools | ||
| 8. external agents | ||
| 9. context configs | ||
| 10. triggers | ||
| 11. scheduled triggers | ||
| 12. sub-agents | ||
| 13. status components | ||
| 14. agents | ||
| 15. project index | ||
|
|
||
| Skills are not part of `generationTasks`. They are written separately by `generateSkills()` because they emit `SKILL.md` files instead of TypeScript source files. |
There was a problem hiding this comment.
This numbered list documents a specific "current task order" that no longer exists. With the switch to import.meta.glob('./*-generator.ts'), execution order is now alphabetical by filename (agent → artifact-component → context-config → credential → ...). Either update this list to reflect alphabetical order or document that order is intentionally unspecified since generators read from an immutable context and write to non-overlapping paths.
| @@ -0,0 +1,8 @@ | |||
| import type { GenerationTask } from '../generation-types'; | |||
|
|
|||
| export const generationTasks: Record<`./${string}-generator.ts`, GenerationTask<any>> = import.meta | |||
There was a problem hiding this comment.
Minor: GenerationTask<any> — since each generator file already uses satisfies GenerationTask<SpecificType> for internal type safety, the any here is acceptable. Consider unknown instead of any to prevent accidental direct payload access on the record values without going through a specific generator's typed collect/generate contract.
| function asRecord(value: unknown): Record<string, unknown> | undefined { | ||
| if (!value || typeof value !== 'object' || Array.isArray(value)) { | ||
| return; | ||
| } | ||
| return value as Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
asRecord is declared identically in three files: here (private), generation-types.ts (private), and collector-common.ts (exported). stripExtension is duplicated here and in collector-common.ts. Import from collector-common.ts to reduce maintenance surface.
| if ( | ||
| !asRecord(data.subAgents) || | ||
| // @ts-expect-error -- existing runtime behavior | ||
| !Object.keys(data.subAgents).length | ||
| ) { | ||
| return { complete: false, reason: 'no sub-agents defined' }; | ||
| } |
There was a problem hiding this comment.
The @ts-expect-error can be eliminated by using the asRecord return value directly:
| if ( | |
| !asRecord(data.subAgents) || | |
| // @ts-expect-error -- existing runtime behavior | |
| !Object.keys(data.subAgents).length | |
| ) { | |
| return { complete: false, reason: 'no sub-agents defined' }; | |
| } | |
| const subAgents = asRecord(data.subAgents); | |
| if ( | |
| !subAgents || | |
| !Object.keys(subAgents).length | |
| ) { | |
| return { complete: false, reason: 'no sub-agents defined' }; | |
| } |
This avoids suppressing a real type error and matches the pattern used everywhere else in this PR.
| export function collectTemplateVariableNamesFromFields( | ||
| values: Array<string | undefined> | ||
| ): string[] { | ||
| const variables: string[] = []; | ||
| for (const value of values) { | ||
| if (typeof value !== 'string') { | ||
| continue; | ||
| } | ||
| variables.push(...collectTemplateVariableNames(value)); | ||
| } | ||
| return variables; | ||
| } |
There was a problem hiding this comment.
collectTemplateVariableNamesFromFields is functionally identical to collectTemplateVariablesFromValues exported from collector-common.ts — same loop, same filter, same output. Import the shared version to avoid divergence.
|
|
||
| - the output is not TypeScript, | ||
| - generation is async and writes files directly, | ||
| - the new output behaves more like the existing `skill-generator`. |
There was a problem hiding this comment.
References skill-generator but the actual module is skill.ts (not inside generators/, and not suffixed -generator). Update to reference skill.ts or say "the existing skill generator (skill.ts)" to avoid confusion when someone searches the codebase for a file called skill-generator.
Ito Test Report ❌19 test cases ran. 18 passed, 1 failed. ✅ Most pull-v4 routing, batch generation, logic integrity, adversarial hardening, and docs viewport checks behaved as expected. 🔍 One path-based pull flow exposed a reproducible local project-loading defect; code inspection supports a real application bug in the TypeScript module loading path used by ✅ Passed (18)
❌ Failed (1)
Project resolution via --project as local path works and targets that directory – Failed
📋 View Recording |
There was a problem hiding this comment.
Is this a transient spec document or are we keeping this documentation indefinitely? If indefinite, it should be put into the agents-docs
Ito Test Report ❌12 test cases ran. 10 passed, 2 failed. 🔍 Verification confirmed broad pull/generation coverage is working, and identified two actionable security defects in skill generation and template literal handling. These two issues have clear production-code defect paths and should be prioritized. ✅ Passed (10)
❌ Failed (2)
Skill path traversal attempt – Failed
Template/code injection strings in context data – Failed
📋 View Recording |
Ito Test Report ❌19 test cases ran. 17 passed, 2 failed. ✅ Most pull-v4 flows validated successfully, including single-project pull, batch pull, merge behavior, safety checks, and docs/workflow coverage. 🔍 Two failures remain as likely real product bugs after code inspection: JSON mode does not terminate cleanly, and overwrite behavior is not wired through the CLI path, allowing stale generated fragments to persist. ✅ Passed (17)
❌ Failed (2)
JSON mode outputs normalized payload without generation – Failed
Overwrite mode regenerates deterministic canonical files – Failed
📋 View Recording |
Ito Test Report ❌18 test cases ran. 3 failed, 15 passed. Across 18 test cases, 16 passed and 2 failed, indicating generally stable pull-v4 behavior across batch generation, merge determinism, edge recovery, mobile usability, and adversarial/security scenarios. The key findings are two confirmed production defects: a pre-existing medium-severity issue where inkeep pull --project support-project --json can print valid JSON but hang due to skipping the explicit success exit path, and a high-severity PR-introduced merge bug where reruns can retain malformed existing TypeScript and append regenerated code instead of safely replacing invalid files. ❌ Failed (3)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Pull with malicious skill id '../escape' was contained to adversarial-project/skills/escape with no filesystem writes outside project root. |  |
| Adversarial | Adversarial payload content was emitted as inert file content in generated skill markdown and did not execute during generation. |  |
| Adversarial | Repeated pull reruns completed without corruption; key files stabilized to deterministic hashes after rerun convergence. |  |
| Adversarial | Invalid auth produced safe failure (401/auth error) and output remained redacted without exposing configured secrets. |  |
| Edge | Project with an empty skills map produced no skill artifact files, matching expected behavior. |  |
| Edge | Nested-directory path mode and project-ID mode both targeted the correct output location without cross-contamination. | N/A |
| Edge | After a forced interruption, immediate retry completed cleanly and service health remained stable before and after retry. | N/A |
| Edge | Mobile viewport flow remained usable at 390x844, including email entry and transition to the next auth step. | N/A |
| Logic | After manually moving and renaming the tool export, rerunning pull preserved ./tools/custom/custom-primary and customPrimaryTool references in index.ts. |  |
| Logic | Repeated pulls on a fixture with duplicate tool display names consistently generated duplicate-name/duplicate-name-1 filenames and stable import aliasing. |  |
| Logic | Not a real app bug. The prior block was environment/fixture methodology. Re-execution via existing regression test passed, and source confirms helper fields are internal-only and stripped from emitted output (agents-cli/src/commands/pull-v4/generators/context-config-generator.ts:66-70, 299-306; validated by test assertions in .../introspect/context-config-regressions.test.ts:135-136). |  |
| Logic | Generated agent file maintained declaration-before-top-level-usage ordering across reruns; no pre-declaration symbol usage was introduced. |  |
| Logic | Not a real app bug. Prior blockage was fixture/setup-related. Re-execution confirmed skip logic in runtime for mixed completeness input and code path confirms generation continues with complete agents only (agents-cli/src/commands/pull-v4/generation-types.ts:92-106,108-129; introspect-generator.ts:31-57). |  |
| Happy-path | Batch pull created the new project directory with skills/general-gameplan/SKILL.md as expected. |
 |
| Happy-path | Project with credential references generated environments/development.env.ts and environments/index.ts. |
 |
Commit: e4d9c94
Tell us how we did: Give Ito Feedback
2 participants
No description provided.