fix: allow concept names as camp go navigation targets

[lancekrogers]

Summary

• cgo docs, cgo ai_docs, cgo festivals, etc. now work alongside the shortcut keys (d, ai, f)
• Added CampaignPaths.AsMap() to expose path concept names as a map
• Updated BuildCategoryMappings to accept optional CampaignPaths and merge concept-name mappings alongside shortcut-key mappings (shortcuts win on collision)

Root cause

BuildCategoryMappings only iterated over the shortcuts section of jumps.yaml, which maps abbreviation keys (d, ai, f) to categories. The paths section defines full concept names (docs, ai_docs, festivals) but was never used for navigation resolution. So cgo docs fell through to fuzzy search with no category and failed.

Test plan

• go build ./... passes
• go test ./... passes (all packages)
• New test TestBuildCategoryMappings_WithPaths covers concept name resolution
• Updated completion test expectations for expanded mapping count

[obey-agent]

Verdict: Request Changes

Overview:
The direction is right and it fixes the happy path, but the alias map still does not reflect the effective campaign paths after normalization and defaulting. That leaves partial and legacy jumps.yaml configs with missing concept-name resolution even though the rest of the settings pipeline normalizes them.

Key Findings:

• internal/config/types.go: PathsMap() returns raw YAML state instead of the normalized/defaulted config, so concept-name aliases can still be absent for legacy or partial settings.

What's Done Well:

• The call sites were updated consistently.
• The UX improvement is well scoped and the added tests are pointed at the intended behavior.

Staff Standard:
Not yet. I would want the effective config and the alias map to agree before merging.

[obey-agent]

Verdict: Approve

Follow-up:
The effective-path alias mismatch is fixed. PathsMap now stays aligned with the normalized/defaulted jumps config, so concept-name navigation no longer diverges for legacy or partial settings.

[obey-agent]

Staff-Level Review — Request Changes

Verdict: Request Changes

Overview: The intent is sound and the BuildCategoryMappings(shortcuts, pathsMap) signature change is the right shape. The implementation, however, is leaky at two important seams — map aliasing in SaveJumpsConfig, and incomplete coverage for partial jumps.yaml files at load time. The new unit test sidesteps the real production path, so the bug the PR is meant to prevent can still reach users through LoadJumpsConfig call sites that don't follow up with ApplyDefaults.

Key Findings

1. SaveJumpsConfig aliases the caller's PathsMap. The shallow copy normalized := *cfg shares the map, and normalized.refreshPathsMap() mutates it as a side effect. The block immediately above explicitly deep-copies Shortcuts for exactly this reason — the handling is now inconsistent. The refresh call is also inert for serialization (PathsMap is yaml:"-" and normalized is discarded after yaml.Marshal), so the cleanest fix is to drop normalized.refreshPathsMap() from Save. See inline at internal/config/settings.go:111.

2. LoadJumpsConfig doesn't apply defaults. Partial jumps.yaml → partial cfg.Paths → asMap() skips empty fields → partial PathsMap. Users with an old/hand-edited config hit the same fuzzy-search fallthrough the PR is trying to eliminate. The main LoadCampaignConfig → loadJumps path compensates by calling ApplyDefaults after the loader returns, but the three direct call sites in cmd/camp/navigation/shortcuts.go (518, 732, 855) don't. Move cfg.ApplyDefaults() into LoadJumpsConfig before refreshPathsMap() so the invariant lives in one place. See inline at internal/config/settings.go:86.

3. asMap() doc comment contradicts the code. "Only used when no jumps.yaml is present" is wrong — refreshPathsMap calls it on every load. This matters because the comment sets the wrong mental model for the next maintainer. See inline at internal/config/types.go:160.

4. Test coverage gap. TestJumpsConfigRefreshPathsMapUsesEffectivePaths hand-constructs a JumpsConfig and explicitly calls ApplyDefaults(), which is precisely the step that's missing from the real load path. Please add a test that writes a partial jumps.yaml (e.g. only paths: {docs: ...}) to a tempdir, round-trips it through LoadJumpsConfig, and asserts that both projects and docs resolve — that's the class of regression the PR is meant to prevent.

What's Done Well

• Layering of concept names beneath explicit shortcuts in BuildCategoryMappings is the right precedence and is documented in the function comment.
• Accepting nil for pathsMap keeps the existing call sites and test fixtures ergonomic.
• Preserving custom aliases via the raw-map second yaml.Unmarshal is a tasteful way to let users keep non-standard path keys without adding struct fields.
• Test and production call-site updates are consistent (cfg.Shortcuts(), cfg.PathsMap()), and the completion count assertions were updated correctly to match the new 11+11 mapping fan-out.

Staff Standard

I would not sign off on this as my own work in its current state. The aliasing bug in SaveJumpsConfig is the kind of subtle defect that surfaces as a flaky test or a user-reported "why did my PathsMap grow unexpectedly?" months down the line, and the loader coverage gap undermines the PR's stated goal for a real class of users. Both are fixable in a small follow-up commit; happy to re-review once the three inline comments are addressed.

[obey-agent]

Staff-Level Review — Request Changes

Verdict: Request Changes

Overview: The intent is sound and the BuildCategoryMappings(shortcuts, pathsMap) signature change is the right shape. The implementation, however, is leaky at two important seams — map aliasing in SaveJumpsConfig, and incomplete coverage for partial jumps.yaml files at load time. The new unit test sidesteps the real production path, so the bug the PR is meant to prevent can still reach users through LoadJumpsConfig call sites that don't follow up with ApplyDefaults.

Key Findings

1. SaveJumpsConfig aliases the caller's PathsMap. The shallow copy normalized := *cfg shares the map, and normalized.refreshPathsMap() mutates it as a side effect. The block immediately above explicitly deep-copies Shortcuts for exactly this reason — the handling is now inconsistent. The refresh call is also inert for serialization (PathsMap is yaml:"-" and normalized is discarded after yaml.Marshal), so the cleanest fix is to drop normalized.refreshPathsMap() from Save. See inline at internal/config/settings.go:111.

2. LoadJumpsConfig doesn't apply defaults. Partial jumps.yaml → partial cfg.Paths → asMap() skips empty fields → partial PathsMap. Users with an old/hand-edited config hit the same fuzzy-search fallthrough the PR is trying to eliminate. The main LoadCampaignConfig → loadJumps path compensates by calling ApplyDefaults after the loader returns, but the three direct call sites in cmd/camp/navigation/shortcuts.go (518, 732, 855) don't. Move cfg.ApplyDefaults() into LoadJumpsConfig before refreshPathsMap() so the invariant lives in one place. See inline at internal/config/settings.go:86.

3. asMap() doc comment contradicts the code. "Only used when no jumps.yaml is present" is wrong — refreshPathsMap calls it on every load. This matters because the comment sets the wrong mental model for the next maintainer. See inline at internal/config/types.go:160.

4. Test coverage gap. TestJumpsConfigRefreshPathsMapUsesEffectivePaths hand-constructs a JumpsConfig and explicitly calls ApplyDefaults(), which is precisely the step that's missing from the real load path. Please add a test that writes a partial jumps.yaml (e.g. only paths: {docs: ...}) to a tempdir, round-trips it through LoadJumpsConfig, and asserts that both projects and docs resolve — that's the class of regression the PR is meant to prevent.

What's Done Well

• Layering of concept names beneath explicit shortcuts in BuildCategoryMappings is the right precedence and is documented in the function comment.
• Accepting nil for pathsMap keeps the existing call sites and test fixtures ergonomic.
• Preserving custom aliases via the raw-map second yaml.Unmarshal is a tasteful way to let users keep non-standard path keys without adding struct fields.
• Test and production call-site updates are consistent (cfg.Shortcuts(), cfg.PathsMap()), and the completion count assertions were updated correctly to match the new 11+11 mapping fan-out.

Staff Standard

I would not sign off on this as my own work in its current state. The aliasing bug in SaveJumpsConfig is the kind of subtle defect that surfaces as a flaky test or a user-reported "why did my PathsMap grow unexpectedly?" months down the line, and the loader coverage gap undermines the PR's stated goal for a real class of users. Both are fixable in a small follow-up commit; happy to re-review once the three inline comments are addressed.

[obey-agent]

Staff-Level Review — Request Changes

Verdict: Request Changes

Overview: The intent is sound and the BuildCategoryMappings(shortcuts, pathsMap) signature change is the right shape. The implementation, however, is leaky at two important seams — map aliasing in SaveJumpsConfig, and incomplete coverage for partial jumps.yaml files at load time. The new unit test sidesteps the real production path, so the bug the PR is meant to prevent can still reach users through LoadJumpsConfig call sites that don't follow up with ApplyDefaults.

Key Findings

1. SaveJumpsConfig aliases the caller's PathsMap. The shallow copy normalized := *cfg shares the map, and normalized.refreshPathsMap() mutates it as a side effect. The block immediately above explicitly deep-copies Shortcuts for exactly this reason — the handling is now inconsistent. The refresh call is also inert for serialization (PathsMap is yaml:"-" and normalized is discarded after yaml.Marshal), so the cleanest fix is to drop normalized.refreshPathsMap() from Save. See inline at internal/config/settings.go:111.

2. LoadJumpsConfig doesn't apply defaults. Partial jumps.yaml → partial cfg.Paths → asMap() skips empty fields → partial PathsMap. Users with an old/hand-edited config hit the same fuzzy-search fallthrough the PR is trying to eliminate. The main LoadCampaignConfig → loadJumps path compensates by calling ApplyDefaults after the loader returns, but the three direct call sites in cmd/camp/navigation/shortcuts.go (518, 732, 855) don't. Move cfg.ApplyDefaults() into LoadJumpsConfig before refreshPathsMap() so the invariant lives in one place. See inline at internal/config/settings.go:86.

3. asMap() doc comment contradicts the code. "Only used when no jumps.yaml is present" is wrong — refreshPathsMap calls it on every load. This matters because the comment sets the wrong mental model for the next maintainer. See inline at internal/config/types.go:160.

4. Test coverage gap. TestJumpsConfigRefreshPathsMapUsesEffectivePaths hand-constructs a JumpsConfig and explicitly calls ApplyDefaults(), which is precisely the step that's missing from the real load path. Please add a test that writes a partial jumps.yaml (e.g. only paths: {docs: ...}) to a tempdir, round-trips it through LoadJumpsConfig, and asserts that both projects and docs resolve — that's the class of regression the PR is meant to prevent.

What's Done Well

• Layering of concept names beneath explicit shortcuts in BuildCategoryMappings is the right precedence and is documented in the function comment.
• Accepting nil for pathsMap keeps the existing call sites and test fixtures ergonomic.
• Preserving custom aliases via the raw-map second yaml.Unmarshal is a tasteful way to let users keep non-standard path keys without adding struct fields.
• Test and production call-site updates are consistent (cfg.Shortcuts(), cfg.PathsMap()), and the completion count assertions were updated correctly to match the new 11+11 mapping fan-out.

Staff Standard

I would not sign off on this as my own work in its current state. The aliasing bug in SaveJumpsConfig is the kind of subtle defect that surfaces as a flaky test or a user-reported "why did my PathsMap grow unexpectedly?" months down the line, and the loader coverage gap undermines the PR's stated goal for a real class of users. Both are fixable in a small follow-up commit; happy to re-review once the three inline comments are addressed.

[obey-agent]

Verdict: Approve

Follow-up:
The current head fixes the remaining config-loader issues. LoadJumpsConfig now applies defaults before refreshing the effective alias map, direct callers no longer depend on ad hoc ApplyDefaults() calls, SaveJumpsConfig no longer mutates the caller's runtime-only PathsMap, and config coverage now exercises partial jumps.yaml plus caller-map non-mutation.

[obey-agent]

Staff-Level Re-Review — Approve

Verdict: Approve

Overview: All three inline issues from my prior "Request Changes" review are addressed at 425cbaea, and the fixes are exactly the right shape. The two new tests in settings_test.go (+66 lines) cover precisely the gaps I flagged — partial jumps.yaml round-tripped through the real LoadJumpsConfig path, and a defensive-copy assertion on SaveJumpsConfig's non-mutation of caller state.

How each prior finding landed

1. SaveJumpsConfig aliasing the caller's PathsMap — fixed at its cleanest: the normalized.refreshPathsMap() call is simply removed. Save now does exactly what serialization needs and nothing more. No defensive deep-copy workaround, no subtle aliasing behavior. TestSaveJumpsConfig_DoesNotMutateCallerPathsMap nails this directly by constructing a cfg with pre-populated PathsMap, calling Save, then asserting the caller's map was not mutated (specifically that it did NOT gain the projects default from Save's side). Good, targeted coverage.

2. LoadJumpsConfig not applying defaults — fixed by swapping cfg.refreshPathsMap() for cfg.ApplyDefaults(), which already calls refreshPathsMap internally as its last step. This collapses two invariants into one call and makes the loader self-sufficient. Consequence: the redundant jumps.ApplyDefaults() calls in campaign.go:loadJumps and internal/project/worktrees_path.go are removed (−1 line each), so the "apply defaults" invariant now lives in one place instead of three. That's architecturally cleaner than my original suggestion. TestLoadJumpsConfig_AppliesDefaultsAndPreservesRawAliases writes a real partial YAML to a tempdir with only intents and a custom documentation alias, round-trips it through LoadJumpsConfig, and asserts: (a) intents gets normalized to .campaign/intents/ by NormalizeIntentNavigation, (b) projects gets populated from ApplyDefaults, and (c) the custom documentation alias survives untouched. That's exactly the "partial jumps.yaml through the real loader" path I asked for.

3. asMap() doc comment contradicting actual usage — fixed. New text: "Returns the canonical concept-name→path map derived from the current CampaignPaths struct values. JumpsConfig.refreshPathsMap uses it to keep the effective alias map aligned with normalized and defaulted runtime paths." That's accurate, names the caller, and sets the correct mental model for the next maintainer.

What's Still Done Well

• The fix uses less code than the previous revision: one call-site removed from Save, two redundant ApplyDefaults calls removed from downstream callers, and the load path collapses to NormalizeIntentNavigation → ApplyDefaults with ApplyDefaults doing the rest. Subtractive.
• Test coverage is targeted at the exact regression classes I flagged, not speculative adjacent cases. Each test is small (~30 lines) and asserts one invariant.
• The asMap doc update names JumpsConfig.refreshPathsMap directly as the caller, so a reader following the code can jump straight to the consumer.

Staff Standard

Approve. This is how review feedback should be addressed: the fix is smaller than the original code, the tests cover the exact gaps I flagged, and the architectural consequence (one-place ApplyDefaults invariant) is better than my original suggestion of "add ApplyDefaults inside Load but leave the downstream calls intact." Ship it.

[obey-agent]

This is not the intended design, the intended design is that these default concepts we include in camp are documented in the yaml file, users can edit that file and the camp go jumping should work with their edits, or whatever is in that file. Jumping to concepts defined in that yaml should work just like with the shortcuts.

[obey-agent]

Addressed the navigation review gaps. camp go now resolves configured shortcut keys, long-form directory aliases, and configured concepts separately, and first-token drill syntax now works via design/... and de@... filesystem traversal with dynamic shell completion.

[obey-agent]

Staff-Level Re-Review — Substantial Architectural Change (still Approve, with two follow-ups)

Verdict: Approve

Commit 3033311 "fix: resolve cgo long-form aliases from config" is a much larger change than the title suggests — 985 lines across 16 files, introducing internal/nav/configured_navigation.go with a new layered resolution architecture. I want to flag that scope growth explicitly since my prior approval was for a narrower fix, and note a couple of observations on the new surface.

What changed

The PathsMap/asMap approach I previously approved has been replaced with a cleaner design:

1. ResolveConfiguredTarget — unified entry point with explicit resolution order: shortcuts → concepts → long-form path aliases.
2. BuildPathAliasMappings — derives aliases from navigation shortcut target paths (e.g. workflow/design/ → design) instead of from a separate PathsMap field. This is a better single-source-of-truth design.
3. NormalizeNavigationName — case-insensitive, trailing-slash-tolerant matching across all three layers.
4. Drill syntax — p/query, p@query patterns handled via splitSlashDrillArgs / splitShortcutDrillArgs.
5. BuildCategoryMappings signature reverted to single-arg (no pathsMap), consistent with the new layering.

Architecturally this is cleaner than what I approved last round. The ambiguity handling in BuildPathAliasMappings (drop the alias rather than silently pick one) is the right default for user-safety.

Observations (non-blocking)

1. JumpsConfig.PathsMap is now dead state. CampaignConfig.PathsMap() was removed in this commit (types.go −33 lines), and the asMap() method was moved to package-private effectivePathsMap in settings.go. But JumpsConfig.PathsMap is still a struct field and refreshPathsMap still populates it. Nothing outside the package reads it anymore. Either delete the field + refreshPathsMap (tidy), or leave a comment explaining why it's kept around for backward-compat (if that's the intent).

2. ResolveConfiguredTarget builds shortcut mappings twice. In the drill-check branch and again in the main branch:

   shortcutMappings := BuildCategoryMappings(cfg.Shortcuts())  // build 1
   // ...
   shortcutMappings := BuildCategoryMappings(cfg.Shortcuts())  // build 2 (identical)

   Same applies to BuildPathAliasMappings being re-derived inside resolveDrillTarget. For a CLI the micro-cost is trivial, but hoisting both to the top of ResolveConfiguredTarget would be cleaner and future-proof if the shortcut map ever grows.

3. PR scope — the PR title is "fix: allow concept names as camp go navigation targets" but this commit adds drill syntax, fuzzy sub-directory matching, and long-form-alias resolution. That's a feature expansion. Consider splitting these into a follow-up PR if the base fix has already proven stable, or updating the PR title/description so reviewers know to expect the broader scope.

What's Still Done Well

• The new code is well-tested: 136 lines of new tests in configured_navigation_test.go plus 30 lines of integration coverage.
• Explicit resolution order documented in the ResolveConfiguredTarget docstring.
• Ambiguity handling is conservative and safe.
• NormalizeNavigationName unifies case/slash handling so there's one canonical normalization across the codebase.
• Shell integration updated (bash, fish, zsh) — the three shells stay in sync.

Staff Standard

Architecture is cleaner than what I approved last round — this is a net improvement. Ship it. The two observations above are follow-up cleanups, not blockers. The PR-scope comment is process feedback for next time, not a request to amend this PR.

[obey-agent]

Design note for the current head (3033311):

This PR now treats camp go / cgo navigation as three distinct sources of names instead of collapsing everything into CampaignPaths.asMap().

1. Shortcut keys from .campaign/settings/jumps.yaml

• Examples: de, ai, p, i
• These still work exactly as before.

2. Long-form directory aliases derived from configured navigation shortcut targets

• Examples: if a configured shortcut points at workflow/design/, then design should work.
• If a configured shortcut points at ai_docs/, then ai_docs should work.
• This is a directory-alias layer, not the concept layer.

3. Configured concepts from .campaign/campaign.yaml

• User-defined concept names should also resolve.
• This covers custom concepts in campaign.yaml instead of only built-in path names.

The intent is to support the UX you described:

• cgo de and cgo design should both work
• cgo ai and cgo ai_docs should both work
• configured concepts in campaign.yaml should work
• shortcuts are preserved; long-form names are added, not substituted

Drill behavior added on this head:

• cgo design/ enters drill mode rooted at workflow/design/
• cgo design/<tab> cycles through actual filesystem entries under that directory
• each additional / drills one level deeper
• cgo de@ is the shortcut form of the same behavior
• cgo de@<tab> cycles entries under the shortcut target
• shortcut drill and slash drill are filesystem-based traversal from the resolved target path, not index-only fuzzy lookup

What changed in code:

• Added a dedicated configured-navigation resolver in internal/nav/configured_navigation.go
• cmd/camp/navigation/go.go now resolves navigation through:
  • shortcut keys
  • long-form directory aliases
  • configured concepts
• internal/complete/complete.go now completes those same names and supports first-token drill completion for design/... and de@...
• shell init scripts were updated so first-token drill completion actually works in zsh/bash/fish
• tests were added/updated for long-form aliases, configured concepts, and drill behavior

Important PathsMap note:

• JumpsConfig.PathsMap is no longer part of runtime navigation resolution on this branch.
• Navigation no longer reads CampaignConfig.PathsMap() and no longer uses CampaignPaths.asMap() as a concept/navigation source of truth.
• PathsMap is currently only maintained inside internal/config/settings.go as an internal effective map for the jumps loader/tests.
• So yes: for navigation, it is effectively dead now.
• I did not fully remove the JumpsConfig.PathsMap field in this PR because that is a separate config cleanup decision, but the navigation design on this head no longer depends on it.

The design line here is:

• shortcuts stay in jumps.yaml
• concepts stay in campaign.yaml
• long-form directory aliases come from configured navigation targets
• those are related, but they are not the same layer anymore

[obey-agent]

Dead code. PathsMap is added to JumpsConfig with ~50 lines of implementation (raw YAML capture, refreshPathsMap, effectivePathsMap) plus ~100 lines of tests, but nothing in production code ever reads it. A repo-wide rg PathsMap only hits settings.go and settings_test.go. Long-form directory aliases are derived from cfg.Shortcuts() via BuildPathAliasMappings, not from PathsMap, so a user-supplied key like paths.documentation: docs/ in jumps.yaml will not create a navigable alias — despite what TestLoadJumpsConfig_AppliesDefaultsAndPreservesRawAliases appears to demonstrate.

If this is intended for future use, land it with the consumer or drop it for now. Shipping a dead field inflates surface area that future maintainers will have to reason about and keep in sync with the rest of the config layer.

[obey-agent]

Dead function. shortcutKeys is no longer called anywhere after the refactor (previously used by Generate and CategoryShortcuts, both of which now call nav.TopLevelNavigationNames directly). rg shortcutKeys shows only the declaration here. Remove it rather than leaving stale helpers — they confuse future readers trying to understand the data flow.

[obey-agent]

Use the project error framework. These three fmt.Errorf call sites (L435, L449, L464) are new production code, and the project's guideline is to wrap via camperrors (you already import it and use camperrors.Wrap on L451). The trailing /-trim formatting is fine, but a plain fmt.Errorf string is not wrappable by callers and loses observability — no type, no sentinel, no errors.Is target.

Minimum: use camperrors.Wrapf(..., "...") or introduce a sentinel like ErrNavigationPathNotFound in internal/errors and wrap it here. This matters because shell completion / scripted callers rely on being able to distinguish 'not found' from 'I/O failed'.

[obey-agent]

Docstring disagrees with implementation. The doc claims the order is (1) shortcut keys → (2) concepts → (3) path aliases, but the actual flow is:

1. Shortcut drill (de@foo) via splitShortcutDrillArgs
2. Shortcut keys (plain de)
3. Slash drill (design/foo) via splitSlashDrillArgs → resolveDrillTarget
4. Concept names (resolveDrillTarget first branch)
5. Path aliases derived from shortcuts (resolveDrillTarget second branch)

Concept names shadow path aliases silently when both exist at the same normalized key. That precedence decision is reasonable but invisible — please encode it explicitly in the docstring so it doesn't get swapped by accident later. Also mention the @ and / drill syntaxes; they're the user-facing surface and should be visible here.

[obey-agent]

Redundant work — compute once, branch twice. BuildCategoryMappings(cfg.Shortcuts()) is rebuilt twice on any input that contains @ (once in the drill branch at L102, then again unconditionally at L114). BuildPathAliasMappings is similarly recomputed inside resolveDrillTarget at L159, and yet a third time inside TopLevelNavigationNames. Each rebuild iterates every shortcut, does path.Base/strings.TrimRight, and allocates a fresh map.

This runs on every tab completion, so it's in a shell-latency-sensitive hot path (shell tab completion is gated on the 200ms complete.Timeout). Compute both maps once at the top of ResolveConfiguredTarget, or memoize them on *CampaignConfig. The ergonomics are also better — you won't have ambiguity collisions diverging between sites.

[lancekrogers]

Normalization inconsistency. Everywhere else in this PR, navigation tokens go through NormalizeNavigationName (lowercase + trim trailing /). Here in completeAll, you roll your own strings.HasPrefix(strings.ToLower(name), strings.ToLower(query)) — which does NOT trim a trailing slash from the query.

Consequence: if a user has typed design/ and ResolveConfiguredTarget hasn't matched (because say concepts list is empty and there's no design path alias), this falls through to completeAll with query="design/". The top-level name stored in topLevelNames is "design" (no slash). Prefix match fails. No suggestions.

Use NormalizeNavigationName on both sides here, or better yet, do the normalization once up front and compare normalized-to-normalized consistently.

[lancekrogers]

Wasted call on short-circuit paths. ResolveConfiguredTarget runs unconditionally here, but the next ~15 lines short-circuit out for the toggle keyword and custom-path navigation shortcuts. For those cases (every cgo t, every cgo foo where foo is a non-standard-path shortcut) we pay the full resolution cost — sort shortcut keys, build both mappings, parse — and throw the result away.

Move the resolved := nav.ResolveConfiguredTarget(cfg, args) call down to just above line 84 where it's actually consumed, so the short-circuits stay cheap.

[lancekrogers]

Errors are swallowed silently. listPathCandidates, listPathCandidatesRich, completeSubdirectoryInPath, and completeSubdirectoryInPathRich all do if err != nil { return nil, nil } on os.ReadDir. That pattern is reasonable for 'missing dir in completion' but it also silently drops permission errors, I/O errors, and bad symlinks. When a user has a corrupted workflow dir or a permission issue, they'll see 'no completions' with zero diagnostic signal.

Differentiate: swallow os.IsNotExist, but bubble up (or at least log to a debug channel / campaign audit log) the real I/O failures. If we care enough to call ctx.Err() inside the loop, we should care enough to distinguish 'nothing to suggest' from 'I/O broke.'

[lancekrogers]

Test name is misleading. This asserts SaveJumpsConfig does not mutate the caller's PathsMap, but it only 'passes' because SaveJumpsConfig never calls ApplyDefaults. ApplyDefaults → refreshPathsMap writes directly into j.PathsMap (settings.go:211-219), so any caller that runs LoadJumpsConfig then ApplyDefaults on their own config would see their PathsMap mutated.

Either (a) rename to TestSaveJumpsConfig_DoesNotApplyDefaults so the actual invariant is clear, or (b) make refreshPathsMap operate on a copy and assign atomically so the non-mutation property holds regardless of call order. Option (b) is friendlier to future callers.

[lancekrogers]

Context cancellation gap. fuzzyResolveDirectory starts with a ctx.Err() check, but the caller resolveRelativePathNavigation does not. So the os.Stat(exactPath) on line 421, the fuzzyResolveDirectory call on line 427, the os.Stat(nestedPath) on line 432 — all proceed even if ctx is already cancelled/timed-out. For the shell-integration path, complete.Timeout is 200ms; a stale call here after timeout hits the disk anyway.

Please add a ctx.Err() gate at the top of resolveRelativePathNavigation and before each os.Stat (at least the second one). Stats are cheap but not free, and the project CLAUDE guidance is 'always check ctx.Err() before starting long operations.'

[obey-agent]

Verdict: Request Changes

Overview

This PR introduces a real improvement — letting users type concept names, long-form directory aliases, and drill syntax (design/foo, de@foo) at the first navigation arg. The core design (dedicated ResolveConfiguredTarget in internal/nav, shared between cgo and the completion engine) is sound and the test coverage for the happy paths is solid. But there are several issues that need cleanup before this ships.

Key Findings

• JumpsConfig.PathsMap is dead code — 50+ lines of implementation plus ~100 lines of test, no production consumer. BuildPathAliasMappings still derives aliases from cfg.Shortcuts(), not from the new PathsMap. See inline on settings.go:34.
• fmt.Errorf in new production paths — violates the project's stated error convention. See inline on go.go:464.
• Dead function shortcutKeys left in complete.go after the refactor. See inline on complete.go:163.
• Docstring in ResolveConfiguredTarget disagrees with the implementation — it describes 3 resolution steps, the code has 5 with non-trivial precedence choices (concept shadows alias, drill-form precedes plain). See inline on configured_navigation.go:94.
• Redundant hot-path work — BuildCategoryMappings and BuildPathAliasMappings recomputed 2–3× per invocation in a shell-completion path gated on a 200ms timeout. See inline on configured_navigation.go:114.
• Normalization inconsistency in completeAll (custom strings.ToLower vs NormalizeNavigationName everywhere else) — breaks completion when a user types a trailing /. See inline on complete.go:255.
• Silent error swallowing in the new listPathCandidates* / completeSubdirectoryInPath* helpers — permission and I/O errors become 'no completions' with no diagnostic signal. See inline on complete.go:357.
• Context-cancellation gap in resolveRelativePathNavigation — does os.Stat without checking ctx.Err(). See inline on go.go:421.
• Misleading test name — TestSaveJumpsConfig_DoesNotMutateCallerPathsMap passes only because SaveJumpsConfig doesn't call ApplyDefaults; any caller running ApplyDefaults directly would see mutation. See inline on settings_test.go:254.
• resolved := nav.ResolveConfiguredTarget(cfg, args) runs before toggle / custom-shortcut short-circuits — wasted work on common paths. See inline on go.go:65.

What's Done Well

• The introduction of a single ResolveConfiguredTarget that both cgo and shell completion share is exactly the right abstraction — no parallel implementation of the resolution logic, which would have been easy to regress.
• NormalizeNavigationName is a clean primitive and it's applied consistently in the new nav package.
• BuildPathAliasMappings correctly drops ambiguous aliases rather than silently picking one — good defensive design.
• Integration tests in tests/integration/navigation_test.go cover the user-facing cgo design, cgo ai_docs, cgo design/festival_app, and cgo de@festival_site flows — those exercises the full stack end-to-end.
• Build passes, all existing tests pass.

Staff Standard

No. Not in this state. The core design is good and I'd be proud of the overall architecture, but shipping a dead PathsMap field with ~150 lines of plumbing, fmt.Errorf in new code after the project explicitly forbids it, and stale helper functions is not staff-level delivery. Tighten the six 'must-fix' items (PathsMap, fmt.Errorf, dead shortcutKeys, redundant computation, completeAll normalization, error swallowing) and this will be ready.