fix(discovery): resolve all native user-scope surfaces from the agent directory (#4769) - #4784
fix(discovery): resolve all native user-scope surfaces from the agent directory (#4769)#4784Yeachan-Heo wants to merge 13 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f0ccee410
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| function resolveUserAgentDir(ctx: LoadContext): string { | ||
| return ctx.userAgentDir ?? getAgentDir(); | ||
| return path.resolve(ctx.userAgentDir ?? getAgentDir()); |
There was a problem hiding this comment.
When createAgentSession({ agentDir }) selects a profile different from the process-wide getAgentDir(), this fallback still selects the process-wide profile because sdk/session.ts invokes loadSkills, rule discovery, and context-file discovery with only cwd, while the system-prompt loaders also expose no agentDir option. The session can therefore omit the requested profile's skills/instructions and inject SYSTEM.md, RULES.md, AGENTS.md, or skills from another profile; pass the resolved session agentDir through all capability-based startup readers rather than relying on this fallback.
Useful? React with 👍 / 👎.
| name: string, | ||
| policy?: SkillsSettings, | ||
| home = getRuntimeHome(), | ||
| agentDir = getAgentDir(), |
There was a problem hiding this comment.
For a session whose explicit agentDir differs from the process-wide directory, SkillDiscoveryTool now advertises skills from the session profile, but SkillTool still calls findRuntimeSkillByName(this.#session.cwd, requestedName, policy) without this new argument. If the skill is not already in the startup snapshot—for example, it was installed during the session—the subsequent invocation searches the global profile and reports the advertised skill as unavailable; thread getSessionAgentDir() into this lookup.
Useful? React with 👍 / 👎.
|
Verification evidence from the originating issue lane (same 22-file change set, original head
Negative control (assertions are not environment-dependent)Reverting only the source diff (tests kept) makes 6/7 of Pre-existing local red, proven unrelated (identical with the diff stashed)
Fail-closed confirmations
Local suites at the original head (all green)
Consolidation audit (no duplicate-lane drift)This change set supersedes and fully contains the Issue-lane handoff (issue #4769 → this PR): ownership of mutation/rebase/review/merge is with |
Keep terminal safety-stop authority exclusive to first-party adapter parse sites and sanitize the trailing completion path before managed-shell rebuilding. Public AI consumers can verify or transfer existing provenance but cannot mint it from message data or structural signals. Fixes the two P1 review findings on #4777. Lore-id: 4777-p1-provenance Constraint: custom provider streams must remain fallback-eligible unless adapter provenance is authenticated Constraint: user cancellation and non-error completion must never become provider safety stops Rejected: public mint helper | arbitrary consumers could forge allowlisted terminal authority Confidence: high Scope-risk: regression-risk Reversibility: revert-commit Tested: focused provider, managed-attempt, session-retry, and provider-safety e2e suites Not-tested: full CI after sibling #4784 merges
|
CI triage for EvidenceI reproduced locally on both the pristine pre-change base ( Why it failsThese tests isolate the user scope by setting only The repair the PR lane should apply (mechanical, same pattern as the suites already updated in this PR)In each affected test, after const originalAgentDir = getAgentDir();
setAgentDir(path.join(home, ".gjc", "agent"));
// … finally: setAgentDir(originalAgentDir)Two tests need slightly more because they deliberately set
Two template/contract failures (also for the PR lane)
|
8f0ccee to
b72820c
Compare
|
Exact-head human gate for PR #4784:
The exact remote #4784 delta was transplanted byte-for-byte, and the follow-up fixes session home/agent-dir injection through actual tool construction so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b72820cb99
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| getActiveSkillPhase: () => session?.getActiveSkillPhase(), | ||
| getDeepInterviewAskStage: () => session?.getDeepInterviewAskStage(), | ||
| getHindsightSessionState: () => session?.getHindsightSessionState(), | ||
| getSessionAgentDir: () => agentDir, |
There was a problem hiding this comment.
When createAgentSession receives profile-scoped settings without a separate agentDir, this accessor returns the process-wide directory captured in agentDir rather than settings.getAgentDir(). This is the path used by canonical subagents: task/executor.ts passes settings: subagentSettings but no agentDir. Consequently, their skill_discovery and runtime skill fallback scan the default profile, omitting the parent's profile skills and potentially advertising skills from another profile. Use the injected settings directory when options.agentDir is absent.
Useful? React with 👍 / 👎.
|
@probepark @snowykr please review exact head |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 3 actionable issues, led by Custom-home runtime discovery ignores the supplied home and Honor the supplied home when resolving the default agent directory. These findings require changes before approval.
Findings / Required Changes
- [P1] Custom-home runtime discovery ignores the supplied home.
Reference:packages/coding-agent/src/extensibility/runtime-skill-discovery.ts:349-350
Whenhomeis provided withoutagentDir, discovery defaults to process-globalgetAgentDir()instead of deriving the default agent directory from that home, so injected/test homes and SDK callers can miss their user skills. Derive the default agent directory from the supplied home or preserve the prior home-relative behavior. - [P1] Honor the supplied home when resolving the default agent directory.
Reference:packages/coding-agent/src/extensibility/runtime-skill-discovery.ts:349-356
When callers provideoptions.homebut omitagentDir, discovery falls back to process-globalgetAgentDir()rather than<home>/.gjc/agent; custom-home callers can therefore read another profile's skills, and concurrent profile changes can select the wrong scope. Derive the default agent directory from the supplied home or require callers to pass both values consistently. - [P2] Document configurable user paths consistently.
Reference:docs/customization.md:45-47
The authority contract and skill documentation identify the configurable agent directory, but the MCP and Hook rows still present ~/.gjc/agent as universally authoritative. Update these rows to use <agentDir> with the default path noted, matching the agent-directory profile behavior.
CI / Verification
- Reviewed the exact remote head:
b72820cb996af0ed19bd17a43bef6342b807b04a. - CI summary: 34 passing, 3 failing, 11 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | Agent-directory profile isolation and legacy default-profile compatibility are established without an actionable API boundary defect. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Runtime skill discovery correctly propagates explicit agent directories, but omitted agentDir with a supplied home can cross profile boundaries; no additional concurrency defect was established. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy risk from agent-directory skill and configuration discovery was reviewed; no actionable regression was established. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 review establishes passing affected tests and CI, with one observable custom-home discovery regression and unresolved PR-contract validation. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | A1/A5 integration and platform documentation is mostly aligned, but MCP and Hook path references remain stale for configured agent-directory profiles. |
Limitations
- PR contract checks failed, so merge-contract compliance is not established; affected validation and tests passed.
- PR contract checks failed, but ci_summary exposes no failure reason, so exact contract noncompliance cannot be assessed.
- PR contract checks failed, so contract validation is not treated as passing evidence.
- Windows native and darwin-arm64 platform jobs were skipped, so platform behavior on those targets was not verified by CI.
|
#4768 author here (the MCP half of #4769's parent). This carries the remaining five surfaces the way the review asked, and the One review item from #4768 is still open, and this PR is the last cheap moment for it. @probepark's approving review there ended with:
This PR keeps I measured the cost on your exact head
16 of 24 are inside files this PR already rewrites, and the 8 others are mechanical — Not blocking from me — I do not own this lane and I am not the reviewer of record. But the request was explicit and the mechanism is being finalized in this diff, so it should be a deliberate accept-or-decline rather than something that falls off the edge of two PRs. (For the record on the other half: #4768's docs deliberately claimed only "discovery, |
|
Reviewer follow-up is complete at exact head Addressed snowykr's three findings:
Please perform a fresh exact-head review and approve if satisfied. The PR remains |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92d7c4f38e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| policy?: SkillManagementPolicy; | ||
| }): Promise<ManagedSkillRecord[]> { | ||
| const home = options.home ?? getRuntimeHome(); | ||
| const agentDir = options.agentDir ?? getAgentDir(); |
There was a problem hiding this comment.
When callers supply the existing home override without the new agentDir option, this now scans the process-wide profile instead of <home>/.gjc/agent; exported SDK consumers and loadCustomizationInventory tests using an isolated home can therefore omit that home's user skills and expose skills from the real profile. Preserve the prior home-only behavior by deriving the default agent directory from an explicitly supplied home, as discoverRuntimeSkills now does.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
First review at exact head 92d7c4f3 — merge blocked. This is real progress on #4767 and I measured it fixing a suite that is currently red on dev, but the session-scoped call sites were not threaded and the test that appears to cover that performs the missing step itself.
measured benefit, not just claimed
I ran the currently-red suites against this head with a real install and natives built:
| suite | on dev ba7096f7 |
on this head |
|---|---|---|
skills.test.ts |
31 pass / 5 fail | 36 pass / 0 fail |
gjc-plugin-no-surface.test.ts |
3 / 1 | 3 / 1 |
coordinator-mcp/stop-session.test.ts |
5 / 6 | 5 / 6 |
So this genuinely closes the skills half of the ambient-home problem. The other two remain red and are not this PR's job.
the trust boundary holds
I checked this first, because a discovery-path change is where a checkout could gain authority. loadCapability selects an explicit caller agentDir or trusted getAgentDir() (capability/index.ts:239-242); project .env overrides are rejected by trustedValue (utils/src/dirs.ts:222-230, :429-432); and Settings.agentDir is constructor state rather than config.yml data (config/settings.ts:544-547). No project-level setting gains a say.
major — session-scoped profiles write to one place and read from another
sdk/session.ts:1288 captures the session's authority:
const agentDir = options.agentDir ?? getDefaultAgentDir();Startup discovery does not use it (:1408-1411):
: logger.time("discoverContextFiles", loadContextFilesResultInternal, { cwd });Only cwd. So a session created with an explicit agentDir writes settings and skills under that profile while loading skills, RULES.md, AGENTS.md, SYSTEM.md and the shared native capability surfaces through the process-wide fallback. That is the same write/read divergence #4767 is about, surviving for session scope precisely because this PR made the readers consume LoadContext.userAgentDir without updating the session call sites that must supply it.
And the test cannot see it. test/issue-4769-user-scope-agent-dir.test.ts:144-169 calls setAgentDir(profile) before loadSkills, which makes the global resolution agree with the session profile — so the divergence is masked by the setup. That is a test performing the missing production step itself: it proves the mechanism can work, not that the session path uses it.
The regression that would catch it: createAgentSession({ agentDir }) while the global getAgentDir() points elsewhere, asserting every startup reader resolves to the session profile.
coverage gaps worth closing with it
Positive pins exist for config, SYSTEM.md, skills, RULES.md and AGENTS.md (test/discovery/builtin-user-agent-dir.test.ts:65-100), and old-path negative controls exist for SYSTEM.md, RULES.md and skills (issue-4769-user-scope-agent-dir.test.ts:76-113) — but not for AGENTS.md, config.yml, or the shared getConfigDirs consumers. Decoys for every migrated surface would make the migration self-guarding.
pre-existing caveat, not attributed here
capability/fs.ts:11-38 collapses every readFile/readDirEntries error into cached absence, and discovery/builtin.ts:67-92 treats an unreadable profile as empty. So a permissions error on a user profile reads as "no configuration" rather than surfacing. Untouched by this PR, worth its own change.
Reviewed by @probepark — method: verified the trust boundary before assessing the fix, measured the currently-red suites against this head with a real install rather than my symlinked worktree, then traced the captured session agentDir into the startup discovery calls and checked what the covering test sets up before asserting.
gajae.pr-review-verdict.v1 merge-blocked sha256:ebfb02cc7ff17126869d7e938dc3719533272a0a7463385bd8186200b3e730d0 reviewer:human reviewer-id:probepark evidence:exact-head-92d7c4f3-fixes-skills-suite-36-0-but-session-scoped-agentdir-is-captured-and-not-threaded-into-startup-discovery
Keep terminal safety-stop authority exclusive to first-party adapter parse sites and sanitize the trailing completion path before managed-shell rebuilding. Public AI consumers can verify or transfer existing provenance but cannot mint it from message data or structural signals. Fixes the two P1 review findings on #4777. Lore-id: 4777-p1-provenance Constraint: custom provider streams must remain fallback-eligible unless adapter provenance is authenticated Constraint: user cancellation and non-error completion must never become provider safety stops Rejected: public mint helper | arbitrary consumers could forge allowlisted terminal authority Confidence: high Scope-risk: regression-risk Reversibility: revert-commit Tested: focused provider, managed-attempt, session-retry, and provider-safety e2e suites Not-tested: full CI after sibling #4784 merges
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 5 actionable issues, led by Ignored home override breaks skill-management API compatibility and Injected home is ignored for skill-management user paths. These findings require changes before approval.
Findings / Required Changes
- [P1] Ignored home override breaks skill-management API compatibility.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:145
resolveNativeSkillScopeDir previously used its third home argument for default user writes, but it is now ignored and user writes use the process-wide agent directory. Preserve the established home-based behavior when agentDir is omitted, while allowing explicit agentDir profiles. - [P1] Injected home is ignored for skill-management user paths.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:145-150,176
resolveNativeSkillScopeDirandlistNativeSkillsForManagementdefaultagentDirto process-globalgetAgentDir()even whenhomeis explicitly supplied. User writes/listing for an injected profile can target or read another profile, especially concurrently. Derive the default agent directory from the injected home, or require/pass agentDir consistently, and add isolation tests for omitted agentDir. - [P1] Injected profile home can expose global user skills.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:179
listNativeSkillsForManagement derives agentDir from process-global getAgentDir() when home is injected but agentDir is omitted, potentially scanning another profile's skills. Derive the default agent directory from the injected home or require an explicit agentDir. - [P1] Injected-home skill writes use the process-global profile.
Reference:packages/coding-agent/src/extensibility/skill-management.ts:145-151
resolveNativeSkillScopeDir ignores its supplied home and defaults agentDir to getAgentDir(), so writeNativeSkill({home}) can write outside the injected profile while discovery resolves that profile’s agent directory; derive the default agent directory from the supplied home or require and consistently pass agentDir. - [P1] Missing ToolSession accessor.
Reference:packages/coding-agent/src/tools/skill.ts:133
The skill tool calls getSessionAgentDir(), but ToolSession does not declare that member. Add the optional accessor to ToolSession (or remove the call) and ensure session construction satisfies the interface.
CI / Verification
- Reviewed the exact remote head:
92d7c4f38ea1fac7dcf2901551d63d7a16289606. - CI summary: 34 passing, 2 failing, 12 pending/cancelled/skipped.
- Failing checks:
Validate exact-head PR contract,PR contract bootstrap. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | Profile-based skill discovery and writing are mostly aligned, but the session accessor contract and backward-compatible home override remain concrete API risks. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Correctness is mostly coherent, but injected-home skill-management calls can violate profile isolation and produce cross-profile results under concurrency. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | Profile isolation is enforced in runtime discovery, but skill-management listing can cross profile boundaries when callers inject home without agentDir. |
| A4. Verification / Tests / CI | APPROVED | A4 review found passing affected tests and no established regression; CI contract failures remain an unresolved verification risk. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and platform review found a profile-isolation risk in the skill writer; documentation changes otherwise align the advertised agent-directory model. |
Limitations
- The PR contract and bootstrap CI checks are failed, so exact-head contract validation could not be established; affected-path tests and virtual integration validation succeeded.
|
Final fix-forward exact head is ready for a fresh review:
Addressed both reviewers' requested changes: actual session startup threading, home-relative skill-management list/write resolution with concurrent isolation, and negative decoys for AGENTS/config/shared config-directory consumers. Affected tests and coding-agent package check pass. Please review and approve this exact head if satisfied. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ebd7b06ea
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ): Promise<ProjectContextFilesResult> { | ||
| const resolvedCwd = options.cwd ?? getProjectDir(); | ||
| const result = await loadCapability(contextFileCapability.id, { cwd: resolvedCwd }); | ||
| const result = await loadCapability(contextFileCapability.id, { cwd: resolvedCwd, agentDir: options.agentDir }); |
There was a problem hiding this comment.
When an SDK embedder calls the exported discoverContextFiles(cwd, profileDir) while the process-wide getAgentDir() points elsewhere, sdk/session.ts still ignores its _agentDir argument and invokes loadContextFilesInternal with only cwd. Consequently this new option remains undefined and the helper reads the global profile's AGENTS.md instead of the requested profile's file. Although createAgentSession now forwards its profile correctly, the standalone SDK discovery helper needs to forward its second argument as well.
Useful? React with 👍 / 👎.
Exact current-head terminal verdict: needs-human
Current-base reconciliation is clean and byte-equivalent to the reviewed change set; latest dev advance was test-only and non-overlapping. Focused 142/0, coding-agent/natives/utils checks, full build, affected-path validation, native builds, state gates, and virtual integration are green. Contract/bootstrap checks are the only failures and intentionally fail closed on The sole remaining blocker is an independent authenticated high-risk APPROVED review from Signed-off: gaebal-gajae (clawdbot) 🦞 |
Allow safe workflow handoffs from manifest terminal states and autoresearch transition phases while preserving ralplan and ultragoal approval guards. Port the preserved regression coverage from the related #4784 work.
f7640c3 to
0e98830
Compare
Newest current-base exact-head refresh
The latest dev movement was workflow/runtime work with no overlap in the PR's 35-file user-scope change set; rebase completed without conflicts and preserved the exact PR diff. Current-base focused suites remain 142 pass / 0 fail, coding-agent/natives/utils checks pass, and full build passes. Prior CI and review evidence is stale after this push; please review this exact head. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
@codex review Review exact head Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e98830240
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = await loadCapability<SlashCommand>(slashCommandCapability.id, { | ||
| cwd: options.cwd, | ||
| agentDir: options.agentDir, | ||
| }); |
There was a problem hiding this comment.
When two SDK sessions share a cwd but use different profile settings, or when the standalone loader receives an explicit agentDir, this call still derives disabledProviders and disabledExtensions from the cwd/process-global capability settings rather than the selected session. After another session initializes that cwd, refreshing or running the first session can therefore enable commands its profile disabled or omit commands it enabled. Extend these options to carry the active settings/policies and forward them to loadCapability, as the capability layer's session-isolation regression path already requires callers to do.
Useful? React with 👍 / 👎.
Allow safe workflow handoffs from manifest terminal states and autoresearch transition phases while preserving ralplan and ultragoal approval guards. Port the preserved regression coverage from the related #4784 work.
0e98830 to
f288412
Compare
Current-base exact-head refresh
The six intervening dev commits are agent/runtime and test changes with no overlap in the PR's 35-file user-scope delta; replay completed without conflicts and preserved the PR diff byte-for-byte. Current-base focused tests are 142/0, migration/trust 77/0, coding-agent/agent/natives/utils checks pass, and full build passes. Previous reviews and CI are stale after this push; please review this exact head. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
@codex review Review exact head Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Digest convention clarification for exact-head evidence: canonical PR digest Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Fresh executor QA receipt for exact head Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
… directory (#4769) Discovery read every native user-scope surface from <home>/.gjc/agent while gjc migrate and gjc skill wrote <agentDir>/skills, so under an agent-directory profile imported user skills landed where nothing scanned them and a SYSTEM.md/RULES.md/AGENTS.md in the directory `gjc config dir` prints was silently never read. One authoritative resolved directory now feeds every reader and writer: builtin provider (skills, SYSTEM.md, RULES.md, AGENTS.md, getConfigDirs surfaces), runtime skill discovery, skill management, and the session/CLI skill_discovery callers, with explicit agentDir injection honored alongside setAgentDir()/GJC_CODING_AGENT_DIR. An agent-directory profile is a separate user scope matching the MCP autoload contract from #4768: the default profile's home-relative roots are not scanned under a profile and vice versa, while the default profile keeps its legacy home-relative skill roots at the documented precedence. A malformed project config.yml no longer hides a valid profile user config. The trusted-home snapshot in loadCapability is untouched and remains the security boundary. Closes #4769. Fixes the remaining non-MCP half of #4767. Lore-id: 4769-user-scope-agent-dir Constraint: must not reopen injectable home in loadCapability (d9fabc8) Constraint: profile user scope must not leak default-profile skills or vice versa Rejected: home-relative user scope | contradicts gjc config dir, gjc migrate, gjc skill writers Rejected: agent dir + always-on home legacy fallback | leaks default profile into every profile and duplicates precedence Tested: bun test discovery/ skills skill-management migrate-cli sdk-skills issue-4508 issue-4769 suites Tested: packages/utils full suite; check-visible-definitions, verify-g002-gates, rebrand-inventory --strict Not-tested: Windows agent-directory profiles Supersedes: worktree gajae-code-issue-4767-remaining (consolidated into this lane) Confidence: high Scope-risk: moderate Reversibility: easy
Skill discovery and skill invocation must follow the session's resolved agent directory and trusted home, not the process-global defaults. This keeps profile writers and readers aligned and prevents stale test or host scope from hiding user skills. Lore-id: 4784-skill-scope Constraint: explicit session agent directories remain authoritative Constraint: foreign and symlinked roots remain fail-closed Tested: focused discovery, skill, and system-prompt suites; coding-agent typecheck Confidence: high Scope-risk: regression-risk Reversibility: revert-commit
Runtime discovery accepted an injected home but silently paired it with the process-global agent directory when callers omitted agentDir. That split could cross profiles and made concurrent custom-home discovery nondeterministic. Reviewer: snowykr requested exact-home derivation, concurrent profile coverage, and configurable MCP/Hook documentation. Constraint: explicit agentDir still wins; omitted agentDir derives <home>/config/agent. Constraint: #4786 auth-broker module constants remain a separate lane. Tested: custom-home and concurrent profile discovery, discovery/skill/MCP/customization suites. Confidence: high Scope-risk: high-risk Reversibility: revert-commit
Session startup captured an explicit agent directory for writes but omitted it from context, system-prompt, skill, and rule discovery. That left profile sessions reading native user surfaces from the process-global scope. Reviewer: probepark identified the session-scoped write/read divergence and the covering test masking it with setAgentDir. Constraint: explicit session agentDir remains authoritative across all native startup readers. Tested: session profile regression, skills, discovery, system-prompt, and coding-agent check. Confidence: high Scope-risk: high-risk Reversibility: revert-commit
System prompt tests left their homedir spy installed across aggregate Bun runs, allowing later PI_CONFIG_DIR assertions to observe a temporary profile. Restore the mock after each case so user-scope tests remain isolated. Tested: system-prompt and PI_CONFIG_DIR suites individually; affected user-scope suites. Confidence: high Scope-risk: regression-risk Reversibility: revert-commit
Skill management still paired an injected home with the process-global agent directory when agentDir was omitted. Centralize home-relative agent resolution so list and write operations stay isolated across profiles, and add explicit decoy and concurrent coverage. Reviewer: probepark and snowykr requested home-relative list/write fixes and negative decoys for config, AGENTS, and shared config-directory consumers. Constraint: explicit agentDir remains authoritative; unreadable-profile behavior stays out of scope. Tested: management, discovery, session, skill, and coding-agent type suites. Confidence: high Scope-risk: high-risk Reversibility: revert-commit
Keep the explicit profile negative-control assertions in repository format. Tested: coding-agent package check. Confidence: high Scope-risk: low-risk Reversibility: revert-commit
Allow safe workflow handoffs from manifest terminal states and autoresearch transition phases while preserving ralplan and ultragoal approval guards. Port the preserved regression coverage from the related #4784 work.
Keep configured agent-directory profiles authoritative when management callers omit an explicit home, retain the public injected-home skill loader seam, and forward session agent scope through standalone context discovery. Restrict session skill loading to the native provider and add decoy regressions for default and injected profiles. Lore-id: 4769-user-scope-agent-dir Constraint: configured agent directories must remain authoritative when home is omitted Constraint: injected SDK homes must not read the process profile Tested: coding-agent check; affected skill, management, discovery, tool, and system-prompt suites Confidence: high Scope-risk: high-risk Reversibility: revert-commit
Derive injected profile scope through the capability registry instead of maintaining a second native-provider path, and isolate the tilde-expansion test from the real home directory. Lore-id: 4769-user-scope-agent-dir Constraint: native skill loading must keep one provider registry path Tested: coding-agent check; focused discovery, skill, and management suites Confidence: high Scope-risk: high-risk Reversibility: revert-commit
Keep the public loadCapability API bound to the trusted process home while routing the SDK home override through the same capability registry and native-provider filtering. This preserves the injected home legacy roots without maintaining a divergent loader path. Lore-id: 4769-user-scope-agent-dir Constraint: injected SDK homes must resolve the same native skill roots as runtime discovery and management Constraint: public loadCapability remains trusted-home bound Tested: coding-agent check; injected-home and user-scope skill suites Confidence: high Scope-risk: high-risk Reversibility: revert-commit
Session cwd rescopes must keep the captured agent directory when reloading context files and skills. Update the skills documentation to name the supported migration writer.
Session-scoped slash commands and native hooks must resolve from the same agent directory as skills and context files. Preserve profile isolation for interactive, print, doctor, and SDK startup paths.
f288412 to
4ceb2ce
Compare
Exact current-base refresh
The 18 intervening dev commits were discovery-adjacent SDK/session test/runtime changes; overlap with this PR is limited to the changelog, and replay completed without conflicts. The PR delta remains 35 files (+983/-204). Current-base focused tests are 142/0, migration/trust 77/0, coding-agent/agent/natives/utils checks pass, and full build passes. Prior review/CI claims are stale after this push. Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
|
@codex review Review exact head Signed-off-by: gaebal-gajae (clawdbot) 🦞 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ceb2ceede
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # import one Codex user skill into your user-wide GJC skills | ||
| mkdir -p ~/.gjc/agent/skills/my-skill | ||
| cp ~/.codex/skills/my-skill/SKILL.md ~/.gjc/agent/skills/my-skill/SKILL.md | ||
| mkdir -p "$(gjc config dir)/skills/my-skill" |
There was a problem hiding this comment.
gjc config dir with gjc config path
In both user-scope installation snippets, gjc config dir is an invalid command: parseConfigArgs rejects every action outside its allowlist, while the CLI help identifies gjc config path as the command that prints the config directory (packages/coding-agent/src/cli/config-cli.ts:127-146,640-645). Consequently the command substitution produces no path, so these examples fail or attempt to write under /skills/my-skill; use $(gjc config path) in the snippets and related guidance.
Useful? React with 👍 / 👎.
4 participants
What
Every native user-scope surface now resolves from one authoritative agent directory (
gjc config dir;--agent-dir/GJC_CODING_AGENT_DIR/setAgentDir()): skills,SYSTEM.md,RULES.md, userAGENTS.md, userconfig.yml, and the sharedgetConfigDirs()surfaces, on both the read and the write side. Closes #4769; fixes the remaining non-MCP half of #4767.Consolidates and supersedes the
gajae-code-issue-4767-remaininglane (its diff was the same product direction; its AGENTS.md surface,getConfigDirsfold, CLI/tool threading, and end-to-endloadCapabilitytest — including the malformed-project-config case — are folded in here).Why (evidence on dev before this PR)
gjc migrate(user)getAgentDir()/skills<home>/.gjc/agent/skillsgjc skill(user)<home>/.gjc/agent/skillsSYSTEM.md/RULES.md/ userAGENTS.mdgjc config dirpoints<home>/.gjc/agentMeasured:
GJC_CODING_AGENT_DIR=<profile> gjc migratereported imported user skills into a directory nothing on the read path scanned, andskills.test.tshad 5 red cases ondevthat are exactly this mismatch (the old reader leaked the real machine's bundled skills into exact-list assertions — a security suite reading real user state). #4768 already made MCP follow the agent directory because every MCP writer did; the same is true here forgjc migrate, andgjc config dirtells every profile user the profile is their config directory.The contract
resolveUserAgentDir(ctx) = path.resolve(ctx.userAgentDir ?? getAgentDir())— explicit injection honored first, process-wide agent dir as fallback. Readers:discovery/builtin.ts(skills / SYSTEM.md / RULES.md / AGENTS.md / getConfigDirs). Writers and CLI/runtime readers:skill-management.ts,runtime-skill-discovery.ts,tools/skill-discovery.ts,tools/skill.ts,sdk/session.ts,cli/skills-cli.ts(session home/agent-dir threaded through).<agentDir>/skillsis scanned — the default profile's home-relative roots are not read into a profile, and a profile's skills never leak into the default profile. No silent duplicate/precedence ambiguity across profiles.<config>/agent/skills><config>/skills>~/.gjc/skills) are still scanned at their documented precedence, exactly as before..gjc/config.ymlproduces a warning; the profile's valid userconfig.ymlstill loads.loadCapability's trusted-home snapshot (d9fabc8f5a, hardened by fix(utils): resolve the authoritative home at call time #4772/test(utils): pin the trusted-home independent-evidence rule on NSS-less identities #4779) is unchanged; no injectable home was reopened.ctx.homestays the injection point for foreign Claude/Codex scans. A hostile mocked HOME injects nothing under a profile (pinned by test).docs/skills.md,docs/customization.md), theskill_discoverytool prompt + regenerated catalog, the settings-schema description, and the regenerated config schema all state the one contract.Risk classification
low-riskregression-riskhigh-riskTesting
test/issue-4769-user-scope-agent-dir.test.ts(7 cases): SYSTEM.md and RULES.md read from the profile with a home decoy unread; profile-only skill scan; hostile mocked-HOME injection fails closed; default-profile legacy roots still scanned; every writer discovered by every reader (writeNativeSkilluser scope found by management list, runtime discovery, find-by-name, and sessionloadSkills;gjc migrateuser skills found byloadSkills).test/discovery/builtin-user-agent-dir.test.ts(from the consolidated lane): all five native user surfaces end-to-end throughloadCapabilityunderGJC_CODING_AGENT_DIR, plus malformed projectconfig.ymlnot hiding a valid profile config.discovery/(145 pass),skills/skill-management/skill_discovery/skill/System prompt (161 pass),migrate-cli/action-planner/import/schema-matrix/redteam(135 pass), MCP autoload/config/CLI/discovery suites (78 pass),sdk-skills,skill-hook-agent-dir-trust,native-skill-hook-cache,customization-extensions,customization-dashboard— all green. The previously redtools/skill-discovery.test.ts7-case root-scope failure is green after session home/agent-dir threading. Reviewer-requested custom-home fallback and concurrent profile-isolation regressions are also green.packages/utilsfull suite 376 pass (trusted-home hardening intact).bun --cwd=packages/coding-agent run check,check:schemas, docs-index/public-sync,check-visible-definitions,verify-g002-gates,rebrand-inventory --strict,default-gjc-definitions.test.ts, PR contract tests — clean. The local rootbun run checkreaches SDK closure after all affected tests pass but reports pre-existing canonicalization violations on the current dev MCP entrypoint graph; the affected CI root-check is the authoritative integration result.Review follow-up
Snowykr's exact-head review requested three changes: derive the default runtime agent directory from an injected home, add concurrent profile-isolation coverage, and document configurable MCP/Hook paths. All three are implemented in the final head above. Issue #4785 is covered by the existing explicit agent-directory test seam in
skills.test.ts; #4786 is a separate pre-existingpackages/aiauth-broker module-constant issue and is not mixed into this PR.Review follow-up
The exact-head reviews found three seams and all are now fixed: session startup passes its captured
agentDirinto context, system-prompt, skill, and rule discovery; runtime and skill-management list/write paths derive<home>/<configDirName>/agentwhen a supplied home omitsagentDir; and the profile regression suite includes negative decoys for AGENTS/config/shared config-directory consumers plus concurrent isolation.ToolSession.getSessionAgentDiris declared and populated by session construction. Unreadable-profile error collapse remains a separate pre-existing caveat.#4785 is covered by the explicit skill-loader/session agent-dir test seam and is not duplicated here. #4786 remains a separate pre-existing
packages/aiauth-broker module-constant issue.GJC verdict
dev4ceb2ceede93b51844743b9e056b86fbf3c338a6(PR-owned commits rebased onto exact currentorigin/dev3cda2e4c99557e3bbf686663f1a9ae5cc0801db9)343067ab8f9ab75f427e275f411ca1ed57814ea03cda2e4c99557e3bbf686663f1a9ae5cc0801db9## [Unreleased]Fix-forward exact-head verification
4ceb2ceede93b51844743b9e056b86fbf3c338a63cda2e4c99557e3bbf686663f1a9ae5cc0801db9sha256:782d117c521bd366a983384c71ad30fed3862606d4008aad8a03f4da48b6dd34agentDirto context-file and skill rediscovery; corrected the docs command name.mcpManagerunavailable under plugin-bundle scenarios) and are outside the 30-file PR diff.Signed-off-by: gaebal-gajae (clawdbot) 🦞
Current-base reconciliation (exact head)
origin/devat3cda2e4c99557e3bbf686663f1a9ae5cc0801db9; intervening dev changes replayed cleanly with no semantic overlap; the co-touched generated tool catalog contains only upstream task-tier additions.4ceb2ceede93b51844743b9e056b86fbf3c338a6sha256:782d117c521bd366a983384c71ad30fed3862606d4008aad8a03f4da48b6dd34Signed-off-by: gaebal-gajae (clawdbot) 🦞