This repository was archived by the owner on Mar 30, 2026. It is now read-only.
feat(AGE-123): Wire identity loader into Pulumi stack - #94
Merged
Conversation
added 3 commits
February 17, 2026 01:43
- Add IdentityManifest and IdentityResult interfaces to cli/types.ts - Create cli/lib/identity.ts with fetchIdentity() supporting Git URLs, repo#subfolder syntax, and local paths with caching - Add 11 unit tests covering local paths, nested files, optional fields, missing/malformed identity.json, type validation errors - Add vitest as dev dependency and update test script
- Add identity? and identityVersion? fields to AgentDefinition - Mark soulContent/identityContent as @deprecated - Add validateAgentDefinition() with mutual exclusivity checks - Sync ManifestAgent in index.ts with new fields - Add 9 unit tests for validation logic
- Import fetchIdentitySync in index.ts for identity-based agents - Add identity-aware workspace file loading alongside preset path - Pull displayName, emoji, volumeSize defaults from identity manifest - Use linearRouting from identity manifest instead of hardcoded map - Add fetchIdentitySync export to identity.ts for sync contexts - Backward compat: preset-based agents unchanged
Contributor
📝 WalkthroughWalkthroughThis PR introduces identity-based agent configuration, enabling agents to fetch their identity from local paths or Git repositories instead of presets. New type definitions, a dedicated identity-loading module, comprehensive validation logic, and integration into the core agent generation flow are added, along with test coverage for validation and identity loading. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent Config
participant Core as Core Engine
participant Identity as Identity Module
participant GitFS as Git/Local Source
participant Workspace as Workspace Generator
Agent->>Core: AgentDefinition (identity or preset)
Core->>Core: Determine config type
alt Identity-based
Core->>Identity: fetchIdentity(source, cacheDir)
Identity->>GitFS: Fetch from Git/Local
GitFS-->>Identity: Files + manifest
Identity->>Identity: Validate manifest
Identity-->>Core: IdentityResult
Core->>Workspace: Generate with identity vars<br/>(emoji, displayName, volumeSize)
else Preset-based
Core->>Workspace: Generate from preset
else Custom (no identity/preset)
Core->>Workspace: Generate base + soulContent
end
Workspace-->>Core: Agent workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
stepandel
commented
Feb 17, 2026
stepandel
left a comment
OwnerAuthor
There was a problem hiding this comment.
🔍 QA Review — APPROVED
Build: ✅ tsc passes clean
Tests: ✅ All 20 tests pass (identity + validate-agent)
Acceptance Criteria
- ✅
index.tsresolves identity viafetchIdentitySync()whenagent.identityis set - ✅ Backward compat: preset agents still use
loadPresetFiles() - ✅
linearActiveActionsByRolefalls back to identity manifest'slinearRouting - ✅
volumeSize,displayName,emojipulled from identity manifest as defaults - ✅ Template vars still processed via
processTemplates() - ⏭️
pulumi preview— cannot verify without Pulumi/AWS credentials, code paths look correct - ✅ Added
fetchIdentitySync()for Pulumi context (sync-only resource construction) - ✅ Both AWS and Hetzner code paths updated with identity-aware vars
Minor note
agentVolumeSize = agent.volumeSize ?? identity.manifest.volumeSize — since volumeSize is required (non-optional number) in AgentDefinition, the ?? fallback will never trigger. Consider making it optional in the type if identity defaults should be possible. Not a blocker.
Ship it 🚢
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cli/lib/__tests__/validate-agent.test.ts (1)
71-75: Test doesn't validate anything — it only checks object properties.This test creates an agent object and checks that properties exist on it, but never calls
validateAgentDefinition. It's testing JavaScript object spread behavior, not the validation function. Either callvalidateAgentDefinitionto confirm it accepts deprecated fields, or remove this test.Proposed fix
it("preserves deprecated soulContent/identityContent fields", () => { - const agent = makeAgent({ soulContent: "# Soul", identityContent: "# Identity" });- expect(agent.soulContent).toBe("# Soul");- expect(agent.identityContent).toBe("# Identity");+ const agent = makeAgent({ preset: null, soulContent: "# Soul", identityContent: "# Identity" });+ expect(() => validateAgentDefinition(agent)).not.toThrow();+ expect(agent.soulContent).toBe("# Soul");+ expect(agent.identityContent).toBe("# Identity"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/lib/__tests__/validate-agent.test.ts` around lines 71 - 75, The test currently only constructs an agent via makeAgent and asserts properties, but never exercises validateAgentDefinition; update the test to call validateAgentDefinition(agent) (or await if async) and assert it returns/does not throw (or returns the normalized definition) and that the returned/validated object still contains soulContent and identityContent; reference makeAgent and validateAgentDefinition to locate the code and adjust the assertions accordingly (or alternatively remove the test if you prefer not to validate deprecated fields).cli/lib/identity.ts (1)
186-188: Async function wraps synchronous code — misleading API.
fetchIdentityis declaredasyncbut calls the synchronous_fetchIdentityinternally. This is misleading since callers might expect I/O to be non-blocking. If the intent is to support future async operations (e.g., async Git operations), consider documenting this. Otherwise, either make the implementation truly async or remove the async wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/lib/identity.ts` around lines 186 - 188, The fetchIdentity function is declared async but simply returns the synchronous _fetchIdentity result, which is misleading; either remove the async keyword and return the synchronous value directly (change export async function fetchIdentity(...) to export function fetchIdentity(...)) or make the implementation truly asynchronous by updating _fetchIdentity to return a Promise and awaiting it inside fetchIdentity (e.g., convert _fetchIdentity to an async function or wrap its result in Promise.resolve). Update any callers/tests if their expectations change and keep the function signature IdentityResult | Promise<IdentityResult> consistent with the chosen approach.cli/lib/__tests__/identity.test.ts (1)
32-35: Fragile parent directory resolution using"..".Using
join(full, "..")to derive the parent directory is fragile. Ifpathis a simple filename without a directory component, this may not behave as expected. Consider usingdirnamefrom thepathmodule for clarity and correctness.Proposed fix
+import { join, dirname } from "path";-import { join } from "path"; ... const full = join(dir, path); - mkdirSync(join(full, ".."), { recursive: true });+ mkdirSync(dirname(full), { recursive: true }); writeFileSync(full, content);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/lib/__tests__/identity.test.ts` around lines 32 - 35, The loop building test files uses join(full, "..") to compute the parent dir which is fragile; change it to use path.dirname for correctness: compute full as join(dir, path) and call mkdirSync(dirname(full), { recursive: true }) before writeFileSync(full, content); ensure dirname is imported from the path module and update references to join/full/mkdirSync/writeFileSync accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/lib/identity.ts`:
- Around line 70-78: The removal step after a failed git pull does not check the
exit code, so capture("rm", ["-rf", repoDir]) should have its result inspected
before attempting to clone: call capture for rm into a distinct variable (e.g.,
rmResult), verify rmResult.exitCode === 0 and if not throw an Error that
includes rmResult.stderr and repoDir, so the re-clone is only attempted when the
directory was actually removed and failures report the real cause; keep the
existing clone logic (clone variable) unchanged aside from this pre-check.
- Around line 155-158: Validate the linearRouting structure on obj before
casting to IdentityManifest["linearRouting"]: add a runtime check (e.g., an
isValidLinearRouting helper) that ensures obj.linearRouting is an object with
expected properties (like add/remove as arrays of strings or whatever the
IdentityManifest defines) and that each entry has the correct types, then only
assign instance.field linearRouting = obj.linearRouting when the validator
passes; otherwise set linearRouting to undefined (or a safe default). Update the
construction that currently does linearRouting: obj.linearRouting as
IdentityManifest["linearRouting"] to use this validation helper and reject or
sanitize malformed inputs to avoid downstream runtime errors.
In `@cli/types.ts`:
- Around line 110-131: The validation currently references agent.name before
confirming it exists; in validateAgentDefinition, move the check if
(!agent.name) { throw new Error(...) } to the top of the function so all
subsequent errors can safely interpolate agent.name; keep the same error text
but ensure the name presence is validated first (i.e., perform the
required-field check before the preset/identity/soulContent and identityVersion
checks).
In `@index.ts`:
- Line 382: The fallback for agentVolumeSize never uses
identity.manifest.volumeSize because ManifestAgent.volumeSize is required;
change the logic so identity defaults can apply: either make volumeSize optional
on ManifestAgent (remove required typing) so agent.volumeSize can be undefined
and keep the expression agent.volumeSize ?? identity.manifest.volumeSize ?? 30,
or keep the type and change the assignment to explicitly treat a sentinel (e.g.,
null/0/ -1) as "unset" by checking agent.volumeSize for that sentinel and then
falling back to identity.manifest.volumeSize or 30; update ManifestAgent
type/name and the variable assignment (agentVolumeSize, agent.volumeSize,
identity.manifest.volumeSize) accordingly.
---
Nitpick comments:
In `@cli/lib/__tests__/identity.test.ts`:
- Around line 32-35: The loop building test files uses join(full, "..") to
compute the parent dir which is fragile; change it to use path.dirname for
correctness: compute full as join(dir, path) and call mkdirSync(dirname(full), {
recursive: true }) before writeFileSync(full, content); ensure dirname is
imported from the path module and update references to
join/full/mkdirSync/writeFileSync accordingly.
In `@cli/lib/__tests__/validate-agent.test.ts`:
- Around line 71-75: The test currently only constructs an agent via makeAgent
and asserts properties, but never exercises validateAgentDefinition; update the
test to call validateAgentDefinition(agent) (or await if async) and assert it
returns/does not throw (or returns the normalized definition) and that the
returned/validated object still contains soulContent and identityContent;
reference makeAgent and validateAgentDefinition to locate the code and adjust
the assertions accordingly (or alternatively remove the test if you prefer not
to validate deprecated fields).
In `@cli/lib/identity.ts`:
- Around line 186-188: The fetchIdentity function is declared async but simply
returns the synchronous _fetchIdentity result, which is misleading; either
remove the async keyword and return the synchronous value directly (change
export async function fetchIdentity(...) to export function fetchIdentity(...))
or make the implementation truly asynchronous by updating _fetchIdentity to
return a Promise and awaiting it inside fetchIdentity (e.g., convert
_fetchIdentity to an async function or wrap its result in Promise.resolve).
Update any callers/tests if their expectations change and keep the function
signature IdentityResult | Promise<IdentityResult> consistent with the chosen
approach.
Comment on lines
+70
to
+78
| const result = capture("git", ["pull", "--ff-only"], repoDir); | ||
| if (result.exitCode !== 0) { | ||
| // Pull failed (e.g., diverged) — re-clone | ||
| capture("rm", ["-rf", repoDir]); | ||
| const clone = capture("git", ["clone", "--depth", "1", url, repoDir]); | ||
| if (clone.exitCode !== 0) { | ||
| throw new Error(`Failed to clone ${url}: ${clone.stderr}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Unchecked rm exit code before re-clone attempt.
When git pull fails, the code attempts to remove and re-clone. However, the rm -rf exit code isn't checked. If removal fails (e.g., permissions), the subsequent clone will fail with a confusing error about the directory already existing.
Proposed fix
if (result.exitCode !== 0) {
// Pull failed (e.g., diverged) — re-clone
- capture("rm", ["-rf", repoDir]);+ const rmResult = capture("rm", ["-rf", repoDir]);+ if (rmResult.exitCode !== 0) {+ throw new Error(`Failed to remove cached repo at ${repoDir}: ${rmResult.stderr}`);+ }
const clone = capture("git", ["clone", "--depth", "1", url, repoDir]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constresult=capture("git",["pull","--ff-only"],repoDir); | |
| if(result.exitCode!==0){ | |
| // Pull failed (e.g., diverged) — re-clone | |
| capture("rm",["-rf",repoDir]); | |
| constclone=capture("git",["clone","--depth","1",url,repoDir]); | |
| if(clone.exitCode!==0){ | |
| thrownewError(`Failed to clone ${url}: ${clone.stderr}`); | |
| } | |
| } | |
| constresult=capture("git",["pull","--ff-only"],repoDir); | |
| if(result.exitCode!==0){ | |
| // Pull failed (e.g., diverged) — re-clone | |
| constrmResult=capture("rm",["-rf",repoDir]); | |
| if(rmResult.exitCode!==0){ | |
| thrownewError(`Failed to remove cached repo at ${repoDir}: ${rmResult.stderr}`); | |
| } | |
| constclone=capture("git",["clone","--depth","1",url,repoDir]); | |
| if(clone.exitCode!==0){ | |
| thrownewError(`Failed to clone ${url}: ${clone.stderr}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/lib/identity.ts` around lines 70 - 78, The removal step after a failed
git pull does not check the exit code, so capture("rm", ["-rf", repoDir]) should
have its result inspected before attempting to clone: call capture for rm into a
distinct variable (e.g., rmResult), verify rmResult.exitCode === 0 and if not
throw an Error that includes rmResult.stderr and repoDir, so the re-clone is
only attempted when the directory was actually removed and failures report the
real cause; keep the existing clone logic (clone variable) unchanged aside from
this pre-check.
Comment on lines
+155
to
+158
| instanceType: typeof obj.instanceType === "string" ? obj.instanceType : undefined, | ||
| skills: obj.skills as string[], | ||
| linearRouting: obj.linearRouting as IdentityManifest["linearRouting"], | ||
| templateVars: obj.templateVars as string[], |
Contributor
There was a problem hiding this comment.
Missing validation for linearRouting structure before casting.
linearRouting is cast to IdentityManifest["linearRouting"] without validating its structure. If the JSON contains linearRouting: "invalid" or linearRouting: { add: 123 }, it will be accepted but may cause runtime errors downstream when the consumer expects arrays.
Proposed fix to add linearRouting validation
+ // Validate linearRouting structure if present+ if (obj.linearRouting !== undefined) {+ if (typeof obj.linearRouting !== "object" || Array.isArray(obj.linearRouting)) {+ throw new Error(`identity.json: "linearRouting" must be an object`);+ }+ const lr = obj.linearRouting as Record<string, unknown>;+ if (lr.add !== undefined && !Array.isArray(lr.add)) {+ throw new Error(`identity.json: "linearRouting.add" must be an array`);+ }+ if (lr.remove !== undefined && !Array.isArray(lr.remove)) {+ throw new Error(`identity.json: "linearRouting.remove" must be an array`);+ }+ }+
return {
name: obj.name as string,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/lib/identity.ts` around lines 155 - 158, Validate the linearRouting
structure on obj before casting to IdentityManifest["linearRouting"]: add a
runtime check (e.g., an isValidLinearRouting helper) that ensures
obj.linearRouting is an object with expected properties (like add/remove as
arrays of strings or whatever the IdentityManifest defines) and that each entry
has the correct types, then only assign instance.field linearRouting =
obj.linearRouting when the validator passes; otherwise set linearRouting to
undefined (or a safe default). Update the construction that currently does
linearRouting: obj.linearRouting as IdentityManifest["linearRouting"] to use
this validation helper and reject or sanitize malformed inputs to avoid
downstream runtime errors.
Comment on lines
+110
to
+131
| export function validateAgentDefinition(agent: AgentDefinition): void { | ||
| if (agent.preset && agent.identity) { | ||
| throw new Error( | ||
| `Agent "${agent.name}": "preset" and "identity" are mutually exclusive. Use one or the other.` | ||
| ); | ||
| } | ||
| if (!agent.preset && !agent.identity && !agent.soulContent) { | ||
| throw new Error( | ||
| `Agent "${agent.name}": must specify either "preset", "identity", or custom content ("soulContent").` | ||
| ); | ||
| } | ||
| if (agent.identityVersion && !agent.identity) { | ||
| throw new Error( | ||
| `Agent "${agent.name}": "identityVersion" requires "identity" to be set.` | ||
| ); | ||
| } | ||
| if (!agent.name) { | ||
| throw new Error(`Agent definition missing required field "name".`); | ||
| } |
Contributor
There was a problem hiding this comment.
Validation order: agent.name used in errors before being validated.
The validation uses agent.name in error messages (lines 112, 118, 124) before checking if name is defined (line 129). If name is undefined, the early error messages will show undefined as the agent name. Move the !agent.name check to the top.
Proposed fix
export function validateAgentDefinition(agent: AgentDefinition): void {
+ if (!agent.name) {+ throw new Error(`Agent definition missing required field "name".`);+ }+
if (agent.preset && agent.identity) {
throw new Error(
`Agent "${agent.name}": "preset" and "identity" are mutually exclusive. Use one or the other.`
);
}
...
- if (!agent.name) {- throw new Error(`Agent definition missing required field "name".`);- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| exportfunctionvalidateAgentDefinition(agent: AgentDefinition): void{ | |
| if(agent.preset&&agent.identity){ | |
| thrownewError( | |
| `Agent "${agent.name}": "preset" and "identity" are mutually exclusive. Use one or the other.` | |
| ); | |
| } | |
| if(!agent.preset&&!agent.identity&&!agent.soulContent){ | |
| thrownewError( | |
| `Agent "${agent.name}": must specify either "preset", "identity", or custom content ("soulContent").` | |
| ); | |
| } | |
| if(agent.identityVersion&&!agent.identity){ | |
| thrownewError( | |
| `Agent "${agent.name}": "identityVersion" requires "identity" to be set.` | |
| ); | |
| } | |
| if(!agent.name){ | |
| thrownewError(`Agent definition missing required field "name".`); | |
| } | |
| exportfunctionvalidateAgentDefinition(agent: AgentDefinition): void{ | |
| if(!agent.name){ | |
| thrownewError(`Agent definition missing required field "name".`); | |
| } | |
| if(agent.preset&&agent.identity){ | |
| thrownewError( | |
| `Agent "${agent.name}": "preset" and "identity" are mutually exclusive. Use one or the other.` | |
| ); | |
| } | |
| if(!agent.preset&&!agent.identity&&!agent.soulContent){ | |
| thrownewError( | |
| `Agent "${agent.name}": must specify either "preset", "identity", or custom content ("soulContent").` | |
| ); | |
| } | |
| if(agent.identityVersion&&!agent.identity){ | |
| thrownewError( | |
| `Agent "${agent.name}": "identityVersion" requires "identity" to be set.` | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/types.ts` around lines 110 - 131, The validation currently references
agent.name before confirming it exists; in validateAgentDefinition, move the
check if (!agent.name) { throw new Error(...) } to the top of the function so
all subsequent errors can safely interpolate agent.name; keep the same error
text but ensure the name presence is validated first (i.e., perform the
required-field check before the preset/identity/soulContent and identityVersion
checks).
| // Pull defaults from identity manifest (agent-level overrides take precedence) | ||
| agentEmoji = identity.manifest.emoji ?? agentEmoji; | ||
| agentDisplayName = agent.displayName || identity.manifest.displayName; | ||
| agentVolumeSize = agent.volumeSize ?? identity.manifest.volumeSize ?? 30; |
Contributor
There was a problem hiding this comment.
Identity volumeSize fallback will never be used.
agent.volumeSize is typed as number (required) in ManifestAgent, so it will always have a value. The fallback chain agent.volumeSize ?? identity.manifest.volumeSize ?? 30 will always use agent.volumeSize, never reaching the identity manifest default. If the intent is to allow identity manifests to provide defaults, consider making volumeSize optional on ManifestAgent when identity is set, or explicitly check for a sentinel value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` at line 382, The fallback for agentVolumeSize never uses
identity.manifest.volumeSize because ManifestAgent.volumeSize is required;
change the logic so identity defaults can apply: either make volumeSize optional
on ManifestAgent (remove required typing) so agent.volumeSize can be undefined
and keep the expression agent.volumeSize ?? identity.manifest.volumeSize ?? 30,
or keep the type and change the assignment to explicitly treat a sentinel (e.g.,
null/0/ -1) as "unset" by checking agent.volumeSize for that sentinel and then
falling back to identity.manifest.volumeSize or 30; update ManifestAgent
type/name and the variable assignment (agentVolumeSize, agent.volumeSize,
identity.manifest.volumeSize) accordingly.
stepandel
commented
Feb 17, 2026
stepandel
left a comment
OwnerAuthor
There was a problem hiding this comment.
🔍 QA Review — PASS
✅ Build passes, all 20 tests pass
✅ Verified against AGE-123 acceptance criteria:
index.tsresolves identity from Git URL viafetchIdentitySync- Backward compat: preset-based agents still use
loadPresetFiles() linearRoutingpulled from identity manifestvolumeSize,displayName,emojipulled from identity manifest with agent-level overrides- Template vars still processed correctly
Ready for merge.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Feb 17, 2026
Merged
This was referenced Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integrates the identity loader into the Pulumi deployment pipeline so agents can be deployed from identity repos instead of built-in presets.
Changes
index.ts: Identity-aware workspace file loading in the agent deployment loopagent.identityis set → usesfetchIdentitySync()to load filesdisplayName,emoji,volumeSizedefaults from identity manifestlinearRoutingfrom identity manifest instead of hardcoded role mappreset-based agents work exactly as beforecli/lib/identity.ts: AddedfetchIdentitySync()export for Pulumi's sync contextBackward Compatibility
Existing manifests with
preset: "pm"etc. are completely unchanged.Depends on: #92 (AGE-121), #93 (AGE-122)
Closes AGE-123
Summary by CodeRabbit
New Features
Tests
Chores