feat(cli): forge try — instant-gratification onboarding (#350)

[initializ-mk]

Implements #350: a single forge try command that takes a first-time user from install to talking to a working agent whose loop they can watch, in under 60 seconds, with zero required decisions.

Stacked on #351 (keyless weather skill). The first commit (fix(skills): weather … wttr.in) belongs to #351 and dedupes on rebase once #351 lands on main — the demo hero needs a keyless weather skill. Review the seven forge try commits.

What it does

$ forge try
  Using OpenAI (signed in).
  Agent: quickstart · skills: weather · tools: http_request, datetime_now, math_calculate

you › what's the weather in Tokyo, should I pack an umbrella?
  ▸ tool   weather_current(location=Tokyo)
  ▸ egress wttr.in   ✓ allowed
  ◂ 18C, light rain this evening
agent › 18C in Tokyo with light rain tonight. Yes, take the umbrella.
  audit  {"tools":["weather_current"],"egress":["wttr.in:allow"]}

The differentiator: the agent's own audit stream (tool calls, egress checks, guardrail blocks) renders inline — the same signal the enterprise story rests on, surfaced as first-run delight.

Design (per the plan, locked)

• In-process, not HTTP. No A2A server, daemon, or port binding. A new runtime.LocalSession assembles the same coreruntime.LLMExecutor forge run uses (builtin tools + vendored skills, egress-enforced client + subprocess proxy, audit + progress hooks, provider client) minus the server/scheduler/MCP/memory. No second executor — a trimmed bootstrap around the shared sub-builders; Run() is untouched (zero risk to the server path).
• Ephemeral by default. Scaffolds into a temp dir cleaned on exit; --keep writes ./forge-quickstart. No secrets on disk for the ephemeral run.
• Reuses scaffold() via two initOptions fields (OutputDir, Preset) — no forked init logic.
• Credential auto-resolution layered over existing resolution: flags → env key (ANTHROPIC>OPENAI>GEMINI) → saved OpenAI OAuth → local Ollama → picker (TTY) → actionable error (no TTY).
• Renderer consumes existing audit events only (no new constants); lives in forge-cli, degrades to plain text (NO_COLOR / non-TTY). --quiet hides it, --audit shows full NDJSON.

Phases (one commit each)

1. Command skeleton + demo preset + ephemeral scaffold
2. Model credential auto-resolution
3. In-process REPL driving the executor
4. Visible-loop renderer
5. First-run polish + graduation hook
6. Docs: quick-start leads with forge try; pipeline moved to new ship-to-production.md
7. Tests

Verification

• Real turn via a mock OpenAI server (LocalSession makes the LLM call, history accumulates); end-to-end test renders a real datetime_now tool call.
• 12 renderer unit tests (event→line mapping, truncation, quiet, --audit echo, no-color); resolveTryProvider ordering + workspace-lifecycle tests.
• go build ./..., go vet ./..., go test ./forge-cli/... all green; gofmt + golangci-lint clean; docs broken-link check clean; no channels in the quick start.

Out of scope (named, not built)

Hosted metered demo key, forge try --ui, a second demo agent — all deferred per the plan.

[initializ-mk]

Review: forge try — instant-gratification onboarding

Reviewed the seven forge try commits (skipped the leading #351 weather commit — already reviewed on #351; it dedupes on rebase as noted). Strong, well-architected PR; the security-critical claims hold up under tracing. Merge-ready — findings are one cross-PR merge-coordination item and four low/nit polish items.

What I verified holds (the parts that matter)

• Genuinely reuses the shared executor, not a fork. LocalSession assembles the same coreruntime.LLMExecutor via the runner's sub-builders; Store is nil so history stays in memory and nothing persists. The "no second executor, Run() untouched" claim is accurate.
• Egress is actually wired, not just constructed. buildTryEgress builds both the in-process enforced client (installed on ctx via WithEgressClient in RunTurn) and the subprocess proxy, both emitting egress_allowed/egress_blocked. This is exactly the "works in isolation vs. where it's mounted" trap this line of work keeps hitting — avoided here.
• The paste-key secret doesn't leak into skill subprocesses. I chased this: try.go does os.Setenv on the pasted key, but SkillCommandExecutor.Run builds a minimal allowlisted env (clean slice, PATH+HOME, then only the skill's declared env vars + proxy/trace/OTel). The weather skill declares env.required: [], so it gets neither the key nor the ambient env. No leak in the shipped demo.
• Fail-closed where it counts: no credential + no TTY → actionable error + exit 1; ephemeral temp dir removed via defer RemoveAll(base); no secrets on disk for the ephemeral run. CI fully green.

Findings (see inline)

1. Medium (cross-PR merge coordination): buildTryEgress is a fourth copy of the egress-construction pattern, on the pre-#348 signature — compiles today only because #348 isn't merged; when #348 lands this won't compile until updated, and until then forge try silently ignores allowed_private_cidrs.
2. Low: egress-resolve failure falls back to a fully-unenforced http.DefaultClient (fail-open).
3. Low/nit: the paste-key comment claims the key is persisted under --keep, but the code never writes it to disk (the safe behavior) — reconcile comment + graduation text.
4. Nit: --yes is a dead flag (parsed, never read).
5. Nit (UX): Ctrl-C at the you › input prompt doesn't exit — signal.NotifyContext catches SIGINT but scanner.Scan() isn't ctx-aware, so it hangs until Enter/Ctrl-D. Honestly documented (Ctrl-D / /exit), but Ctrl-C is a reflex in a hero demo. Consider reading stdin in a goroutine and selecting on ctx.Done().

Nice work — the audit-stream-as-first-run-delight framing is a genuinely good idea, and the reuse discipline (one executor, trimmed bootstrap) is exactly right.

[initializ-mk]

Medium (cross-PR merge coordination). This block (security.Resolve at L205, NewEgressEnforcer here at L224, NewEgressProxy at L240) is a fourth copy of the egress-construction pattern, and it's on the pre-#348 signature — Resolve 5-arg, NewEgressEnforcer 4-arg, NewEgressProxy 2-arg. It compiles today only because #348 isn't merged yet. The moment #348 lands (which adds allowedPrivateCIDRs to all three), this file won't compile until updated, and until then forge try will silently ignore allowed_private_cidrs where the server path honors it. Since you own both PRs: land #348 first and rebase this onto it (threading allowedPrivateCIDRs through buildTryEgress), or track it as a required follow-up. Also directly reinforces #348's "consolidate the copies" note — this is now the copy most likely to diverge, since its comment says it "mirrors Run()'s egress setup" but it's a hand-maintained duplicate.

[initializ-mk]

Low. On a security.Resolve error this returns http.DefaultClient — a fully unenforced client — with only a Warn. That's a fail-open on a security path. Effectively unreachable for the fixed quickstart preset (its scaffolded egress config resolves fine), but the wrong default direction: a future config change that makes Resolve fail would silently drop egress enforcement entirely. Prefer a deny-all / empty-allowlist enforced client here so the failure mode stays closed.

[initializ-mk]

Low/nit — comment vs. code. This says the pasted key is "written to disk solely under --keep, via the scaffold env path," but it isn't: the key goes into res.EnvOverrides (session-only, consumed by NewLocalSession), while opts.EnvVars is always map[string]string{} — so the key is never persisted, even under --keep. The never-persist behavior is the correct, safe one and matches the PR's "no secrets on disk" promise, so just fix the comment. Worth surfacing the consequence in the graduation text too: forge try --keep + a pasted key writes ./forge-quickstart with no credential, so a later forge serve there needs the env var set by hand.

[initializ-mk]

Nit — dead flag. f.yes is parsed here but never read anywhere in runTry/downstream. It's advertised in --help ("assume yes to prompts") but does nothing. Either wire it (e.g. auto-select the picker's recommended default so --yes is truly non-interactive) or drop the flag.

[initializ-mk]

Addressed the review in a16dc14:

• Finding 2 (Low, fail-open egress) — fixed. buildTryEgress now fails closed on a security.Resolve error: it returns an allowlist enforcer with no allowed domains (blocks all egress) instead of the unenforced http.DefaultClient. A future config that makes Resolve fail can no longer silently drop enforcement.
• Finding 3 (Low/nit, paste-key comment) — fixed. Corrected the comment to state the key is session-only (res.EnvOverrides, consumed by NewLocalSession) and never written to disk, even under --keep, and noted the consequence: a kept agent has no credential on disk, so a later forge serve there needs the env var set by hand.
• Finding 4 (Nit, dead --yes flag) — dropped it (struct field, flag registration, and parse all removed); --help no longer advertises it.
• Finding 5 (Nit/UX, Ctrl-C at prompt) — fixed. tryREPL now reads stdin on a goroutine and selects on ctx.Done() / EOF / the line, so Ctrl-C at you › exits cleanly instead of hanging until Enter.
• Finding 1 (Medium, feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337) #348 coupling) — feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337) #348 is still open, so I can't rebase onto it yet; buildTryEgress compiles against current main. Added an explicit NOTE (#348 coupling) at the construction site flagging that these are the pre-feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337) #348 signatures and must thread allowedPrivateCIDRs when feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337) #348 lands (else forge try would silently ignore allowed_private_cidrs). Tracking it as a required rebase step — will thread the CIDRs through when feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337) #348 merges.

Build/vet/lint clean; cmd + runtime + tryview suites green. Thanks for the careful trace on the egress-mount and paste-key-leak paths.

[initializ-mk]

Re-review: fix commit a16dc140 — all actionable findings resolved

Verified each finding against the fix diff. CI fully green (all 6 builds, Lint, Test, Integration, Doc-link).

Finding 2 (Low — fail-open egress) — ✅ resolved

On security.Resolve error, buildTryEgress now returns a deny-all enforced client (NewEgressEnforcer(nil, ModeAllowlist, nil, allowPrivateIPs) — allowlist mode with no domains blocks everything) instead of the unenforced http.DefaultClient. Fail-closed, exactly as recommended. The allowPrivateIPs computation was correctly hoisted above the error branch so both paths share it.

Finding 4 (dead --yes) — ✅ resolved

Removed from the struct, init(), and parseTryFlags — no longer advertised in --help.

Finding 3 (paste-key comment) — ✅ resolved

Comment rewritten accurately: the key is session-only (res.EnvOverrides, consumed by NewLocalSession), never written to disk even under --keep, and it now spells out the consequence (a kept agent has no on-disk credential; a later forge serve needs the env var set by hand).

Finding 5 (Ctrl-C at the you › prompt) — ✅ resolved, correctly

stdin now reads on a goroutine feeding a lines channel; the REPL selects on ctx.Done() (Ctrl-C → newline + return), eof (Ctrl-D → return), or a line. The one-read goroutine leak on exit is real but harmless (the process is exiting) and honestly acknowledged in the comment. Mid-turn cancellation and graduation-on-exit both still work.

Finding 1 (#348 coupling) — appropriately deferred with an explicit NOTE

You can't rebase onto an unmerged branch, so the right move is what was done: a prominent in-code NOTE on buildTryEgress documenting that the three constructors are on pre-#348 signatures and must thread allowedPrivateCIDRs when #348 lands, cross-referencing #348's consolidation note. Keeps the landmine visible for whoever merges #348 second.

Verdict: all fixes correct, nothing dangling. Merge-ready (modulo the #348 sequencing the NOTE now tracks).

One optional residual, non-blocking and on an effectively-unreachable branch (the fixed preset always resolves): on the resolve-error path the in-process client is now fail-closed, but it still returns proxyURL="", so skill subprocesses would run unproxied rather than blocked. Fully fail-closing that dead branch would mean refusing to start when egress can't resolve — reasonable to leave as-is behind the NOTE.

[initializ-mk]

Alignment review: rebase needed against merged main (#354 + #348 + #355)

#354 (keyless weather skill) is now merged, and so are #348 (allowed_private_cidrs) and #355 (SOCKS5 raw-TCP). This PR isn't mergeable as-is — the rebase needs two specific changes.

1. Drop the stale weather commit fe9623130 — it would revert #354

The first commit here is the #351 weather implementation and modifies the exact files #354 just merged (SKILL.md, both weather-*.sh scripts, egress_step.go, scanner_test.go, .claude/skills/forge.md) with a different implementation (| tonumber, hourly[4], explicit UA vs. the merged hourly[0], string values). On rebase it conflicts, and keeping this side would silently revert the merged #354.

Drop fe9623130 entirely. forge try references the skill by name (Skills: ["weather"]), so it doesn't need to carry any weather changes — main's #354 version supplies it. Verified this is a clean drop: the forge try phase commits (c37ba45f…) touch cmd/try.go, local_session.go, tryview/, docs — none touch weather files.

Compatibility with #354's version confirmed:

• quickstartPreset → Skills: ["weather"] resolves to main's skill.
• jq dependency unchanged — both #351 and #354 declare bins: [curl, jq], so dropping this copy adds no new requirement.
• egress_domains: [wttr.in] flows into buildTryEgress's skill-derived allowlist; the "weather in Tokyo" starter prompt hits wttr.in through the subprocess proxy.
• The e2e test uses datetime_now, not weather, so the #351→#354 script differences don't affect this suite.

2. buildTryEgress is now a hard compile-break (the NOTE came due)

See inline. main now has Resolve(…, allowedPrivateCIDRs, allowedTCP) (7 args), NewEgressEnforcer(…, allowedPrivateCIDRs) (5), NewEgressProxy(…, allowedPrivateCIDRs) (3). buildTryEgress still calls the pre-#348 5/4/2 forms, so after rebase it won't compile at four call sites.

---

Bottom line: not mergeable as-is. Rebase must (1) drop fe9623130 so it doesn't revert #354, and (2) update buildTryEgress to the merged signatures. After that, forge try uses main's #354 weather skill cleanly. Happy to re-review once the rebased branch is up.

[initializ-mk]

Compile-break after rebase — the NOTE above came due. Both #348 and #355 are now merged, so main's signatures are:

• Resolve(profile, mode, explicitDomains, toolNames, capabilities, allowedPrivateCIDRs, allowedTCP) — 7 args
• NewEgressEnforcer(base, mode, domains, allowPrivateIPs, allowedPrivateCIDRs) — 5 args
• NewEgressProxy(matcher, allowPrivateIPs, allowedPrivateCIDRs) — 3 args

This block calls the pre-#348 5/4/2 forms, so it won't build at four sites (this Resolve, both NewEgressEnforcer incl. the deny-all path, and NewEgressProxy). Mirror Run():

allowedPrivateCIDRs, _ := security.ParsePrivateCIDRs(egressCfg.AllowedPrivateCIDRs)

egressCfg, err := security.Resolve(
    r.cfg.Config.Egress.Profile, r.cfg.Config.Egress.Mode, domains, nil,
    r.cfg.Config.Egress.Capabilities,
    r.cfg.Config.Egress.AllowedPrivateCIDRs,   // #348
    r.cfg.Config.Egress.AllowedTCP)            // #355
...
denyAll  := security.NewEgressEnforcer(nil, security.ModeAllowlist, nil, allowPrivateIPs, nil)
enforcer := security.NewEgressEnforcer(nil, egressCfg.Mode, egressCfg.AllDomains, allowPrivateIPs, allowedPrivateCIDRs)
proxy    := security.NewEgressProxy(matcher, allowPrivateIPs, allowedPrivateCIDRs)

Thread the CIDRs (not just nil to compile) so forge try actually honors allowed_private_cidrs, per the NOTE. allowedTCP is empty on the keyless demo, so the SOCKS5 listener stays off and no SetTCPMatcher is needed (add it only if you want raw-TCP parity). Delete the NOTE block once done.

[initializ-mk]

Rebased against merged main (#354 + #348 + #355). Both required changes done:

1. Dropped the stale weather commit fe9623130. Rebased the seven forge try phase commits --onto origin/main, dropping the #351 weather implementation entirely — it would have reverted #354. forge try references the skill by name (quickstartPreset → Skills: ["weather"]), so it carries no weather changes; main’s #354 skill supplies it. Verified: the diff vs main now touches zero weather files (SKILL.md, weather-*.sh, egress_step.go, scanner_test.go, forge.md all untouched), and a forge try --keep smoke scaffolds main’s #354 script (hourly[0], not #351’s hourly[4]), with egress correctly deriving wttr.in.

2. Updated buildTryEgress to the merged egress signatures + deleted the NOTE. Mirrored Run():

• Resolve(…, Egress.AllowedPrivateCIDRs, Egress.AllowedTCP) — 7 args (feat(security): allowed_private_cidrs — narrow the private-IP block (PR 1/2 for #337) #348 + feat(security): SOCKS5 raw-TCP egress with port-aware allowlist — closes #337 (PR 2/2) #355)
• ParsePrivateCIDRs(egressCfg.AllowedPrivateCIDRs) once, shared by enforcer + proxy
• NewEgressEnforcer(…, allowedPrivateCIDRs) (5 args) on both the live and the deny-all (fail-closed) paths
• NewEgressProxy(matcher, allowPrivateIPs, allowedPrivateCIDRs) (3 args)

allowedTCP is empty on the keyless demo, so the SOCKS5 listener stays off and registerSkillTools(reg, proxyURL, "") passes an empty socksURL — no raw-TCP parity needed (also picked up registerSkillTools’ new 3rd socksURL arg, which the note didn’t call out). The #348 coupling NOTE block is deleted now that it’s resolved.

forge try now honors allowed_private_cidrs like the server path. Build/vet/lint clean; cmd + runtime + tryview suites green; end-to-end forge try scaffold verified against main’s #354 weather skill. Ready for re-review.

[initializ-mk]

Re-review: rebase aligned with merged main ✅

Both alignment problems from the prior review are resolved, and CI green across all 6 builds is the direct proof the signature break is gone.

Problem 1 (weather-commit conflict with #354) — resolved

• The stale fe9623130 (#351 weather implementation) is gone — no weather-*.sh, SKILL.md, scanner_test.go, or egress_step.go in the diff.
• The one remaining shared file, .claude/skills/forge.md, is a clean addition (just the forge try §15 pointer) — it doesn't touch #354's weather example, so nothing reverts the merged skill.
• forge try now uses main's #354 weather skill purely by reference (Skills: ["weather"]).

Problem 2 (buildTryEgress compile-break) — resolved correctly

• Resolve(...) passes 7 args incl. AllowedPrivateCIDRs (#348) + AllowedTCP (#355).
• Both NewEgressEnforcer calls are 5-arg (deny-all path passes nil; the real one threads the parsed allowedPrivateCIDRs).
• NewEgressProxy(matcher, allowPrivateIPs, allowedPrivateCIDRs) is 3-arg.
• ParsePrivateCIDRs(...) wired like Run(), so forge try now honors allowed_private_cidrs rather than just compiling. The #348 coupling NOTE is removed.
• AllowedTCP passed to Resolve (validation runs); empty on the keyless demo so the SOCKS5 listener stays off — no SetTCPMatcher needed, as expected.

Mergeable, CI fully green (all 6 builds incl. Windows, Lint, Test, Integration, Doc-link). Fully aligned with merged main — nothing reverts #354, and forge try honors the merged egress features (#348/#355). Merge-ready.

(mergeStateStatus: BLOCKED is branch-protection/required-review, not a conflict — MERGEABLE confirms no conflicts.)

[initializ-mk]

Review: latest updates (4 new commits) — three clean, one Low finding

✅ c316de203 — masked paste echo: correct

readMaskedLine echoes • per char (never the actual key characters), handles backspace/Ctrl-C/submit, and falls back to silent term.ReadPassword on non-TTY. Key never hits plaintext/scrollback. providerFromInput now whitelists the paste-key provider (o/a/g or full names) — good hardening.

✅ e549d9014 — no placeholder credential + real error: good

buildEnvVars returns empty for Preset, so forge try no longer scaffolds your-api-key-here into .env where it would shadow the real OAuth/env credential and 401 — correctness fix and consistent with "no secrets on disk." The errBox/OnError capture surfaces the real provider failure (shown in the user's own terminal; endpoint already sanitized per #358, so no key leak).

✅ dbf949d5f — colored logo/picker: cosmetic, no credential-handling change.

⚠️ 820bb083e — forge auth logout: good design, one Low finding

What's right: the agent-runtime guard (FORGE_PLATFORM_TOKEN set OR InContainer() → refuse) is sound, and the doc comment is honest about it being defense-in-depth rather than the primary control (an agent with shell could rm the file; real mitigation is fs perms on ~/.forge). Correct framing.

Finding — Low (path traversal via unvalidated provider arg): see inline. runAuthLogout passes the raw CLI arg to oauth.LoadCredentials/DeleteCredentials, which map it to filepath.Join(dir, provider+".json") unsanitized — so forge auth logout '../../../../tmp/x' deletes /tmp/x.json. Low severity (laptop-only — refuses in container/platform; operator-invoked; .json-suffixed files the user already owns; no priv-esc), but a real input-validation gap and inconsistent with the paste-key path this same PR just hardened. Trivial fix: whitelist provider before it touches the store.

Minor: runAuthLogout ignores the LoadCredentials error (tok, _ :=), so a corrupt cred file reports "nothing to do" and won't be cleared. Edge case, low.

CI fully green. Everything except the traversal gap is solid.

[initializ-mk]

Low (path traversal via unvalidated provider). provider is taken raw from args[0] and passed to oauth.LoadCredentials (L55) and oauth.DeleteCredentials (L59), both of which map it to filepath.Join(dir, provider+".json") with no sanitization (store.go:311, removePlaintextFile is always reached). So:

forge auth logout '../../../../tmp/x'
→ os.Remove(~/.forge/credentials/../../../../tmp/x.json) → deletes /tmp/x.json

Severity is Low — laptop/dev-only (the deniedInAgentRuntime guard blocks the container/platform case, so this isn't agent-reachable), operator-invoked, only .json-suffixed files the user already owns, no priv-esc. But it's a real validation gap and inconsistent with the paste-key path this same PR just hardened with providerFromInput. Close it at this entry point:

if _, ok := providerKeyEnv[provider]; !ok {
    return fmt.Errorf("unknown provider %q (use openai, anthropic, or gemini)", provider)
}

Defense-in-depth, the store.go provider→path helpers should also reject path separators (they're the pre-existing traversal-capable code shared by save/load/remove), but validating here closes the newly-reachable CLI vector. A TestAuthLogout_RejectsTraversalProvider case would pin it.

[initializ-mk]

Addressed the review in d544a5f:

Finding (Low — path traversal via unvalidated provider) — fixed. runAuthLogout now whitelists provider against providerKeyEnv (openai/anthropic/gemini — the same set the paste-key path uses) before the value can reach the store’s filepath.Join(dir, provider+".json") mapping. So forge auth logout ../../../../tmp/x now returns unknown provider "../../../../tmp/x" instead of deleting /tmp/x.json. Added TestAuthLogout_RejectsTraversalProvider covering the ../, embedded-slash, unknown, and non-LLM (google) cases.

Minor (ignored LoadCredentials error) — fixed. It now only reports "nothing to do" on a definitive empty (err == nil && tok == nil); on a read error (corrupt token file) it falls through to DeleteCredentials so logout still clears it rather than silently leaving it in place.

On your defense-in-depth note: I scoped this fix to the CLI entry point (the newly-reachable vector), as you suggested. The deeper hardening — having the oauth store’s provider→path helpers (save/load/removePlaintextFile, shared with forge init and MCP login) reject path separators at the source — is pre-existing shared forge-core code, so I’d rather not widen this forge-try PR into it. Happy to file a follow-up for that store-level guard if you want it tracked.

The three clean commits (masked paste echo, no-placeholder-credential + real-error surfacing, colored logo/picker) are unchanged. Build/vet/lint clean; cmd suite green.

[initializ-mk]

Re-review: fix commit d544a5f3 — both items resolved ✅

Finding (path traversal) — resolved at the entry point. runAuthLogout now validates provider against providerKeyEnv (openai/anthropic/gemini — the same whitelist the paste-key path uses) before it reaches the credential store, so ../../../../tmp/x is rejected with "unknown provider" and never reaches filepath.Join(dir, provider+".json"). Reachable CLI traversal vector closed.

Minor (corrupt creds) — resolved. Short-circuit is now err == nil && tok == nil — "nothing to do" only on a definitive empty store; a read error (corrupt token file) falls through to DeleteCredentials so logout actually clears it.

Test — solid. TestAuthLogout_RejectsTraversalProvider pins traversal (../../../../tmp/x), embedded slash (openai/../../etc/x), real-but-unsupported (google), and junk (x) — all rejected. Good coverage, including the embedded-slash case.

CI fully green (all 6 builds incl. Windows, Lint, Test, Integration, Doc-link).

Verdict: both resolved with the right fix, well-tested. Nothing dangling.

Residual (non-blocking, correctly scoped): the store.go provider→path helpers remain traversal-capable in principle, but every current caller now passes a fixed or validated provider, so there's no reachable untrusted input. Rejecting path separators there would be pure defense-in-depth against a future caller — fine to leave as-is or drop a backlog note. Closing the actual vector at the entry point was the right call.