samples(demo-script-tool): address PR #134 review feedback

codemonkeychris · codemonkeychris · commit 884176ca8804 · 2026-05-04T00:27:02.000-07:00
- StepFileWriter: refuse model-emitted relative paths that escape the step directory (path traversal via "..", absolute paths, drive letters). Resolves and verifies each target is strictly inside stepDir before writing — model output is untrusted, not a sandbox. - StepFileWriter: fall back to the first .cs file when multi-file mode doesn't emit Program.cs, so OutputPath always points at a real file. Without this, alternate entry-point names left primary as the directory and downstream File.Exists checks silently no-op'd (canonicalize-on-write skipped, Open Folder couldn't restore step.Code). - IModelClient + CopilotSdkClient + GithubModelsClient: new ResetAsync method so a long-lived SDK client (CopilotSdkClient caches a started CopilotClient) can be torn down after gh auth refresh. Pipeline calls it in both auth-retry paths — without it, refreshing gh auth was a no-op because the CopilotClient session was authenticated under the old token. GithubModelsClient implements as no-op (it reads the token per-call already). - DemoScriptParser: drop section-0 preamble content rather than capturing it into RawTail. The serializer always writes RawTail at the end, so capturing preamble silently moved any text between H1 and the first H2 below the steps section on the next save. - README: rewrite the auth section to describe the actual Copilot SDK / Copilot CLI flow (gh auth + Copilot-enabled account) instead of the obsolete GITHUB_TOKEN / models:read scope description. Refs: copilot-pull-request-reviewer comments on PR #134.
diff --git a/samples/apps/demo-script-tool/App/Services/CopilotSdkClient.cs b/samples/apps/demo-script-tool/App/Services/CopilotSdkClient.cs
@@ -231,6 +231,41 @@ public async Task<string> DescribeAvailableModelsAsync(CancellationToken ct)
         }
     }
 
+    /// <summary>
+    /// Tear down the cached <see cref="CopilotClient"/> so the next
+    /// <see cref="StreamAsync"/> call re-runs <see cref="GetClientAsync"/>
+    /// and starts a fresh CLI session. The pipeline calls this after
+    /// <c>gh auth refresh</c> — without it the SDK keeps using the
+    /// already-started session that was authenticated under the old token,
+    /// and the auth-retry path silently fails again.
+    /// </summary>
+    public async Task ResetAsync(CancellationToken ct)
+    {
+        await _initLock.WaitAsync(ct).ConfigureAwait(false);
+        try
+        {
+            if (_client is { } client)
+            {
+                System.Diagnostics.Debug.WriteLine("[CopilotSdk] resetting CLI client (auth refresh)");
+                try { await client.StopAsync().ConfigureAwait(false); }
+                catch (System.Exception ex)
+                {
+                    System.Diagnostics.Debug.WriteLine($"[CopilotSdk] StopAsync during reset threw: {ex.Message}");
+                }
+                try { await client.DisposeAsync().ConfigureAwait(false); }
+                catch (System.Exception ex)
+                {
+                    System.Diagnostics.Debug.WriteLine($"[CopilotSdk] DisposeAsync during reset threw: {ex.Message}");
+                }
+                _client = null;
+            }
+        }
+        finally
+        {
+            _initLock.Release();
+        }
+    }
+
     public async ValueTask DisposeAsync()
     {
         if (_client is { } client)
diff --git a/samples/apps/demo-script-tool/App/Services/DemoScriptParser.cs b/samples/apps/demo-script-tool/App/Services/DemoScriptParser.cs
@@ -83,10 +83,15 @@ void FlushCurrentStep()
             switch (section)
             {
                 case 0:
-                    // Pre-section content (between H1 and the first H2). Empty lines OK.
-                    // Anything substantial is captured into RawTail to keep round-trips honest.
-                    if (!string.IsNullOrWhiteSpace(raw))
-                        rawTail.AppendLine(raw);
+                    // Pre-section content between the H1 title and the first H2.
+                    // Capturing into rawTail would round-trip incorrectly: the
+                    // serializer always writes rawTail at the end of the file,
+                    // which would silently move preamble below the steps section
+                    // on the next save. The format spec doesn't reserve space for
+                    // a preamble, so we drop section-0 content rather than
+                    // reorder it. (If a real round-trip need shows up, switch
+                    // to a separate RawHeader field that the serializer emits
+                    // immediately after the H1.)
                     break;
 
                 case 1:
diff --git a/samples/apps/demo-script-tool/App/Services/GenerationPipeline.cs b/samples/apps/demo-script-tool/App/Services/GenerationPipeline.cs
@@ -171,6 +171,11 @@ async Task<bool> StreamSingleStepWithAuthRetryAsync(DemoScriptModel model, strin
             System.Diagnostics.Debug.WriteLine($"[Pipeline] AuthExpired on step {step.Number}, retrying after re-auth: {ex.Message}");
             _status.SetGeneratingStatus("Re-authenticating with GitHub…");
             await _auth.EnsureAuthenticatedAsync(ct).ConfigureAwait(false);
+            // Drop any cached client state (e.g. CopilotSdkClient's long-lived
+            // CopilotClient session) so the retry actually picks up the new
+            // gh auth credentials. Without this, refreshing gh auth state
+            // changes nothing the next StreamAsync call can see.
+            await _client.ResetAsync(ct).ConfigureAwait(false);
             _status.SetGeneratingStatus($"Generating step {step.Number} of {model.Steps.Count}…");
             return await StreamSingleStepAsync(model, projectRoot, step, prior, ct).ConfigureAwait(false);
         }
@@ -376,6 +381,12 @@ async Task<bool> ApplyFixAttemptAsync(StepModel step, DemoScriptModel model, str
         catch (AuthExpiredException)
         {
             await _auth.EnsureAuthenticatedAsync(ct).ConfigureAwait(false);
+            // Same reason as in the streaming-retry path above: refreshing gh
+            // auth alone doesn't help an SDK that cached a long-lived session.
+            // We still return false here (the outer build-and-fix loop will
+            // count this as a consumed attempt), but at least the NEXT fix
+            // attempt will start from a fresh client.
+            await _client.ResetAsync(ct).ConfigureAwait(false);
             return false;
         }
 
diff --git a/samples/apps/demo-script-tool/App/Services/GithubModelsClient.cs b/samples/apps/demo-script-tool/App/Services/GithubModelsClient.cs
@@ -131,5 +131,14 @@ public async IAsyncEnumerable<string> StreamAsync(
         return null;
     }
 
+    /// <summary>
+    /// No-op: this client reads the GitHub token via <see cref="GhAuth.GetTokenAsync"/>
+    /// at the start of every <see cref="StreamAsync"/> call, so a fresh
+    /// <c>gh auth refresh</c> is picked up automatically. The interface
+    /// requires the method for SDK-backed clients that snapshot credentials
+    /// (see <see cref="CopilotSdkClient.ResetAsync"/>).
+    /// </summary>
+    public Task ResetAsync(CancellationToken ct) => Task.CompletedTask;
+
     public void Dispose() => _http.Dispose();
 }
diff --git a/samples/apps/demo-script-tool/App/Services/IModelClient.cs b/samples/apps/demo-script-tool/App/Services/IModelClient.cs
@@ -1,5 +1,6 @@
 using System.Collections.Generic;
 using System.Threading;
+using System.Threading.Tasks;
 
 namespace DemoScriptTool.App.Services;
 
@@ -22,6 +23,17 @@ public interface IModelClient
     /// the underlying HTTP read when <paramref name="ct"/> is cancelled.
     /// </summary>
     IAsyncEnumerable<string> StreamAsync(string systemPrompt, string userPrompt, CancellationToken ct);
+
+    /// <summary>
+    /// Drop any cached auth state so the next <see cref="StreamAsync"/> call
+    /// re-authenticates. Called by the pipeline after a successful
+    /// <c>gh auth refresh</c> so an SDK that snapshots its credentials at
+    /// startup (e.g. <see cref="CopilotSdkClient"/>'s long-lived
+    /// <c>CopilotClient</c>) actually picks up the new ones. Implementations
+    /// that read the token per-call (<see cref="GithubModelsClient"/>) can
+    /// no-op.
+    /// </summary>
+    Task ResetAsync(CancellationToken ct);
 }
 
 /// <summary>Thrown when the GitHub Models API rejects the call as unauthenticated.</summary>
diff --git a/samples/apps/demo-script-tool/App/Services/StepFileWriter.cs b/samples/apps/demo-script-tool/App/Services/StepFileWriter.cs
@@ -44,8 +44,14 @@ public string Write(int stepNumber, IReadOnlyDictionary<string, string> files, s
         // Multi-file mode: write each file under step-NN/ verbatim.
         var stepDir = Path.Combine(projectRoot, $"step-{stepNumber:D2}");
         Directory.CreateDirectory(stepDir);
+        // Used to validate every model-supplied path stays inside stepDir —
+        // see comment on TryResolveContainedPath. The trailing separator on
+        // the canonical form is what makes the StartsWith check below safe
+        // against the "stepDir" / "stepDir-evil" prefix-collision case.
+        var stepDirCanonical = Path.GetFullPath(stepDir).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + Path.DirectorySeparatorChar;
 
         string? primary = null;
+        string? firstCsFile = null;
         bool sawCsproj = false;
 
         foreach (var kv in files)
@@ -56,14 +62,25 @@ public string Write(int stepNumber, IReadOnlyDictionary<string, string> files, s
             if (rel.StartsWith(prefix, System.StringComparison.OrdinalIgnoreCase))
                 rel = rel[prefix.Length..];
 
-            var target = Path.Combine(stepDir, rel);
+            // SECURITY: model output is untrusted. A path like "../../foo" or an
+            // absolute path joined with stepDir would let a generated step
+            // overwrite arbitrary files under the user's workspace. Reject any
+            // path that, once resolved, doesn't sit strictly inside stepDir.
+            if (!TryResolveContainedPath(stepDirCanonical, rel, out var target))
+            {
+                System.Diagnostics.Debug.WriteLine($"[StepFileWriter] rejecting unsafe path '{kv.Key}' → outside step dir");
+                continue;
+            }
+
             Directory.CreateDirectory(Path.GetDirectoryName(target)!);
             WriteWithRetry(target, kv.Value);
 
             if (rel.EndsWith(".csproj", System.StringComparison.OrdinalIgnoreCase))
                 sawCsproj = true;
             if (rel.EndsWith("Program.cs", System.StringComparison.OrdinalIgnoreCase))
                 primary ??= target;
+            else if (firstCsFile is null && rel.EndsWith(".cs", System.StringComparison.OrdinalIgnoreCase))
+                firstCsFile = target;
         }
 
         if (!sawCsproj)
@@ -72,7 +89,45 @@ public string Write(int stepNumber, IReadOnlyDictionary<string, string> files, s
             WriteWithRetry(fallbackCsproj, ScaffoldCsproj());
         }
 
-        return primary ?? stepDir;
+        // Prefer Program.cs as the entry point (it's what the system prompt
+        // asks for), but fall back to the first .cs we wrote so the canonical
+        // OutputPath points at a real file. Without this fallback, when the
+        // model picks an alternate name (App.cs, Demo.cs) the step's
+        // OutputPath would be the directory and downstream
+        // `File.Exists(primary)` checks all silently no-op — the canonical
+        // step.Code never gets refreshed from disk and Open Folder can't
+        // restore the file's content on relaunch.
+        return primary ?? firstCsFile ?? stepDir;
+    }
+
+    /// <summary>
+    /// Resolve <paramref name="rel"/> relative to <paramref name="stepDirCanonical"/>
+    /// and verify it stays inside the step directory. Returns false for paths
+    /// that would escape (<c>..</c> traversal, absolute paths, junctions, etc.)
+    /// so the caller can refuse to write them.
+    /// </summary>
+    static bool TryResolveContainedPath(string stepDirCanonical, string rel, out string target)
+    {
+        target = string.Empty;
+        if (string.IsNullOrEmpty(rel)) return false;
+        // Path.IsPathRooted catches absolute paths AND paths with drive letters
+        // ("C:\foo") — both should be refused even though Combine would happily
+        // discard stepDir and use the rooted side as-is.
+        if (Path.IsPathRooted(rel)) return false;
+
+        try
+        {
+            var combined = Path.GetFullPath(Path.Combine(stepDirCanonical, rel));
+            if (!combined.StartsWith(stepDirCanonical, System.StringComparison.OrdinalIgnoreCase))
+                return false;
+            target = combined;
+            return true;
+        }
+        catch (System.Exception ex)
+        {
+            System.Diagnostics.Debug.WriteLine($"[StepFileWriter] path resolve failed for '{rel}': {ex.Message}");
+            return false;
+        }
     }
 
     /// <summary>
diff --git a/samples/apps/demo-script-tool/README.md b/samples/apps/demo-script-tool/README.md
@@ -1,7 +1,7 @@
 # Demo Script Tool
 
 A WinUI 3 desktop sample app built on Reactor. Author a coding demo as a
-single Markdown file (`demo-script.md`) and let the GitHub Models AI SDK
+single Markdown file (`demo-script.md`) and let the GitHub Copilot SDK
 generate runnable .NET code for each step plus speaker notes you can paste
 into your slides.
 
@@ -29,15 +29,27 @@ dotnet run --project samples/apps/demo-script-tool/App/DemoScriptTool.csproj --
 (Reactor's own `--devtools` flags pass straight through; the first
 non-flag argument is treated as the folder path.)
 
-On first launch the app will look up your GitHub token via:
+### Authentication
 
-1. `$env:GITHUB_TOKEN` (set this for non-interactive use), or
-2. `gh auth token` (the GitHub CLI's cache), or
-3. A spawned `gh auth login --web --scopes models:read` flow, which opens an
-   interactive console window — this is the recommended route.
+The app uses the [`GitHub.Copilot.SDK`](https://www.nuget.org/packages/GitHub.Copilot.SDK)
+NuGet package, which talks to a bundled Copilot CLI. That CLI piggybacks on
+whatever account `gh auth` currently considers active, so:
 
-The token is read at the moment of each generation call; it is not cached
-inside the app.
+1. Make sure you have the [GitHub CLI](https://cli.github.com/) installed and
+   on `PATH`.
+2. Sign in with a Copilot-enabled account: `gh auth login --web`.
+3. Confirm with `gh auth status` — the active account needs to have GitHub
+   Copilot Pro / Pro+ / Business / Enterprise so the SDK can issue Copilot
+   chat completions on your behalf.
+
+The default model is `claude-sonnet-4.5`; change it by passing a different
+id to the `CopilotSdkClient` constructor in `DemoScriptShell`. Use the
+**⚡ Devtools → Log available Copilot models…** menu item to see what model
+ids your account can use.
+
+If a generation call hits an authentication failure mid-stream, the pipeline
+runs `gh auth login` (in a spawned console) and resets the cached Copilot
+SDK client so the retry picks up the refreshed credentials.
 
 ## 2. Author a demo
 

Original file line number	Diff line number	Diff line change
`@@ -131,5 +131,14 @@ public async IAsyncEnumerable<string> StreamAsync(`
`131`	`131`	`return null;`
`132`	`132`	`}`
`133`	`133`
	`134`	`+ /// <summary>`
	`135`	`+ /// No-op: this client reads the GitHub token via <see cref="GhAuth.GetTokenAsync"/>`
	`136`	`+ /// at the start of every <see cref="StreamAsync"/> call, so a fresh`
	`137`	`+ /// <c>gh auth refresh</c> is picked up automatically. The interface`
	`138`	`+ /// requires the method for SDK-backed clients that snapshot credentials`
	`139`	`+ /// (see <see cref="CopilotSdkClient.ResetAsync"/>).`
	`140`	`+ /// </summary>`
	`141`	`+ public Task ResetAsync(CancellationToken ct) => Task.CompletedTask;`
	`142`	`+`
`134`	`143`	`public void Dispose() => _http.Dispose();`
`135`	`144`	`}`