fix: prevent security scan from flagging its own grep patterns#7
Merged
sytone merged 4 commits intoApr 21, 2026
Merged
Conversation
Use variable interpolation for the private key grep pattern so the workflow file no longer contains the literal strings it searches for. This eliminates the false positive on security-scan.yml itself. The two secret-handling SKILL.md files contain documentation examples of key headers — these are not modified as they are Squad-managed template content.
Filter out .copilot/skills/secret-handling/ and .squad/templates/skills/secret-handling/ paths which contain documentation examples of private key headers.
The skills haven't moved to .copilot/ yet (PR sytone#5 pending), so the .github/ path also needs to be excluded.
This is a dev-time default in appsettings.json, not a production concern for the repo. Production config is handled separately.
sytone
added a commit
that referenced
this pull request
May 26, 2026
#563) * feat(domain): introduce SubAgentSpawnMode = Embody | Mirror DU (#562 step 1/5) Phase 5 / F-6 step 3, first staged commit of the migration to clarify sub-agent spawn intent at the type level. Today SubAgentSpawnRequest is a bag of optional fields where the "embody a role" path (Archetype + SystemPromptOverride + ModelOverride + ApiProviderOverride + ToolIds + Name) shares record-shape with the "mirror an existing named agent" path (TargetAgentId), and precedence is implicit in DefaultSubAgentManager. This commit introduces SubAgentSpawnMode as a closed discriminated union (abstract sealed record with exactly two derivations: Embody and Mirror) and threads an OPTIONAL Mode property onto SubAgentSpawnRequest so the new shape lands side-by-side with the old fields. No production code is rewired yet — that lands in commits 2 and 3. All old top-level fields on SubAgentSpawnRequest remain UNCHANGED; they are deleted and Mode becomes `required` in commit 5. Design decisions (locked via ask_user after rubber-duck design critique): 1. Embody is a strict pass-through of role + optional customisations. Role-derived defaults (per-role tool sets, prompts) are deferred to #467 — EmbodyCustomizations carries no derivation logic and every field is nullable with a `static readonly Default = new()` singleton for the common "no overrides" case. 2. Embody.Role is validated against a closed set of the 6 known SubAgentArchetype statics (Researcher, Coder, Planner, Reviewer, Writer, General). The smart-enum's open FromString registry stays intact for back-compat at other call sites, but the Embody record itself cannot smuggle unknown roles past construction. Validation runs from the [JsonConstructor] so deserialised payloads receive the same gate as direct construction. 3. Mirror is `record Mirror(AgentId TargetAgentId)` — strict, no metadata overrides. This is a deliberate behaviour change from today: the current code silently passes Name / ModelOverride / Archetype / ToolIds through to the mirrored child even though the intent is "mirror the target". Mirror clients that previously supplied any of these fields will be rejected by the tool layer in commit 2 with an explicit mode-mixing error. 4. The JSON discriminator property is "mode" with values "embody" and "mirror", declared via [JsonPolymorphic] + [JsonDerivedType]. No production boundary serialises SubAgentSpawnRequest today (ServiceBus uses InboundMessage envelopes; IInternalTrigger uses InternalTriggerRequest) — the annotations are defensive hygiene so future serialisation paths inherit the closed shape. 19 unit tests cover construction (Default singleton identity, all-null customisation pin, 6-role closed-set Theory, unknown-role rejection, null-arg rejection, Mirror construction), JSON round-trip both modes including full customisations, and rejection of missing / unknown discriminators. Note the missing-discriminator path raises NotSupportedException (not JsonException) — this is the contract STJ exposes for polymorphic abstract bases without a discriminator and is pinned by name. Out of scope for this commit: - SubAgentSpawnTool still constructs the old fields (commit 2). - DefaultSubAgentManager.SpawnAsync still pattern-matches on the old fields (commit 3). - Test callsites still use the old shape (commit 4). - Old fields and the architecture fence land in commit 5. Closes a piece of #562; remainder follows in the staged commit story. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(gateway): SubAgentSpawnTool builds Mode + rejects mode-mixing (#562 step 2/5) Phase 5 / F-6 step 3, second staged commit. The tool's agent-facing JSON shape is preserved exactly — the same flat object with task / name / model / apiProvider / tools / systemPrompt / archetype / targetAgentId — but the tool now translates that flat shape into the closed SubAgentSpawnMode union introduced in commit 1 and rejects mode-mixing. Behavioural changes: 1. The tool populates request.Mode in every call. * targetAgentId present + no embody fields → Mode = Mirror(AgentId.From(targetAgentId)). * targetAgentId present + ANY embody field → ArgumentException with the list of conflicting field names. The error message names every offending field in one shot so the agent can fix the call once, not iteratively. This is a deliberate behaviour change from today's silent pass-through — Mirror is strict. * targetAgentId absent + no embody fields → Mode = Embody(General, EmbodyCustomizations.Default). The Default singleton (same reference for every call with no overrides) keeps allocation at zero for the common case. * targetAgentId absent + any embody field → Mode = Embody(role, new EmbodyCustomizations { ... }) — only allocates when overrides are actually present. 2. The legacy top-level fields (Name / ModelOverride / etc. / TargetAgentId / Archetype) remain populated unchanged. This is deliberate — DefaultSubAgentManager.SpawnAsync is still on the old read path until commit 3. Both shapes are in sync per call so either reader produces identical behaviour during the migration window. Step 5 deletes the old shape atomically with making Mode `required` and removing the synced writes. 3. The unused private helper ReadArchetype was removed; the responsibility moved into the new BuildSpawnMode pipeline via ResolveArchetype (still using SubAgentArchetype.FromString for the open-registry back-compat path — Embody.Role's closed-set guard enforces the closed shape at the type boundary). Mode-mixing rejection design (per locked decision strict_reject_all): the JSON tool layer is the friendliest place to surface the conflict because the call hasn't yet entered the spawn pipeline. Throwing ArgumentException flows through the agent loop's ToolExecutor as an IsError=true tool result with the message body — the calling LLM sees "don't do that, here's why" and can self-correct in the next turn. 10 new unit tests: - Mode = Embody(General, Default) on the bare-minimum {task} call; the Default singleton identity is pinned via ShouldBeSameAs so the no-overrides path stays allocation-free. - Mode = Embody(role, Default) when only archetype is supplied (reviewer → Reviewer). - Mode = Embody(role, custom) with every embody field populated (Coder + name + model + apiProvider + tools + systemPrompt) — pins the full round-trip of every customisation field through the tool layer. - Mode = Mirror(AgentId) when only targetAgentId is supplied; the AgentId.Value is pinned so the Vogen wrapper isn't a no-op. - Mode-mixing rejection: a Theory over name / model / apiProvider / systemPrompt / archetype each pairs targetAgentId with one of the embody fields and pins the rejection message naming both fields. - Mode-mixing rejection: tools array (separate because the value shape differs from the string-Theory above). - Mode-mixing rejection: multiple conflicts in one call are all reported in a single error message (covers the "one error per call, not iterative fix" UX guarantee). Full Gateway.Tests suite: 1861 passed / 1 skipped / 0 failed / 0 warnings under TreatWarningsAsErrors. The legacy assertions on the old top-level fields (SpawnTool_SpawnsSubAgent_WithDefaults etc.) still pass — back-compat is intact. Refs #562. Next: commit 3 (DefaultSubAgentManager.SpawnAsync prefers Mode over the legacy fields). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(gateway): DefaultSubAgentManager.SpawnAsync pattern-matches on Mode (#562 step 3/5) Phase 5 / F-6 step 3 of 5. Routes the production spawn path (DefaultSubAgentManager.SpawnAsync) through the SubAgentSpawnMode discriminated union introduced in step 1 and constructed by SubAgentSpawnTool in step 2. What changed * DefaultSubAgentManager.SpawnAsync: the inline reads of request.Archetype / request.TargetAgentId / request.Name / request.ModelOverride / request.ApiProviderOverride / request.ToolIds / request.SystemPromptOverride are replaced by a single `switch (request.Mode)` that resolves 7 typed locals (archetype, baseDescriptor, childAgentId, name, modelOverride, apiProviderOverride, toolIds, systemPromptOverride) once. * The Embody arm uses the parent descriptor as the base and pins the archetype value into the child agent id role slot: `{parent}--subagent--{archetype}--{uniqueId}`. * The Mirror arm looks up the target descriptor via the agent registry (KeyNotFoundException if absent — fail-fast surfaces misconfiguration rather than silently degrading) and pins the target id into the role slot: `{parent}--subagent--{targetAgentId}--{uniqueId}`. Per the locked design (#562), Mirror is strict pass-through — no embody customisations leak through. * A `case null:` arm preserves the legacy field-population code path exactly so the ~12 existing test callsites that still set the top-level fields keep passing under back-compat through step 4. * A `default:` arm throws InvalidOperationException with the unknown subclass name — guards against silent acceptance if a third SubAgentSpawnMode subtype is added without updating SpawnAsync. * SubAgentInfo.Name and .Model now read from the resolved `name` / `modelOverride` locals (was request.Name / request.ModelOverride), ensuring the typed Mode arms drive what callers see post-spawn. Tests * tests/gateway/BotNexus.Gateway.Tests/Agents/SubAgentSpawnModeRoutingTests.cs NEW. 4 [Fact]s pin the Mode-driven SpawnAsync contract: - Embody clones parent + lands archetype in role slot. - Embody customisations propagate through to SubAgentInfo.Name + .Model (proves typed Mode actually drives the post-spawn surface). - Mirror uses target descriptor base + lands target id in role slot (locked-design pin: differentiates mirror from same-parent role spawn). - Mirror with unknown target throws KeyNotFoundException (fail-fast vs silent-degrade). Validation * Full Gateway.Tests suite: 1865 passed / 1 skipped / 0 failed / 0 warnings under TreatWarningsAsErrors=true (up from 1861 baseline; +4 new mode-routing pins). * Existing SubAgentTargetAgentIdTests + SubAgentToolTests (24 facts total) stay green under the case-null back-compat arm — no regressions introduced. Refs #562 * test(gateway): migrate legacy SubAgentSpawnRequest callsites to set Mode (#562 step 4/5) Phase 5 / F-6 step 4 of 5. Flips every test that calls `manager.SpawnAsync(...)` from setting the optional top-level fields (TargetAgentId / Archetype) to setting the typed `Mode = new Embody(...)` / `Mode = new Mirror(...)` discriminated union introduced in step 1 + threaded through the production paths in steps 2-3. Files migrated (10 callsites across 4 files): * Agents/SubAgentArchetypeIdentityTests.cs — both spawns: - Archetype = Reviewer → Mode = new Embody(Reviewer) - default → Mode = new Embody(General) * Agents/SubAgentEagerConversationPinTests.cs — all 3 spawns: - default → Mode = new Embody(General) * Agents/SubAgentKindTests.cs: - SpawnAsync_MirrorOfNamedAgent_*: TargetAgentId = "researcher-prime" → Mode = new Mirror(AgentId.From("researcher-prime")) - CreateSpawnRequest helper: default → Mode = new Embody(General) * Agents/SubAgentTargetAgentIdTests.cs — all 4 spawns: - TargetAgentId = "farnsworth" → Mode = new Mirror(AgentId.From("farnsworth")) - TargetAgentId = "ghost-agent" → Mode = new Mirror(AgentId.From("ghost-agent")) - no TargetAgentId → Mode = new Embody(General) - TargetAgentId = "farnsworth" (deny-list test) → Mode = new Mirror(...) Out of scope for this step: * Agents/SubAgentModelsTests.cs — pure domain-record reflection / equality tests, no SpawnAsync invocation. Its `RequiredProperties_AreMarkedRequired` test and the two `new SubAgentSpawnRequest` callsites will be updated in step 5 when the legacy fields are deleted and Mode becomes required. Why the SubAgentKindTests Mirror child id slot did NOT need test adjustment: ResolveChildAgentId(registry, spawned.SubAgentId) searches descriptors by substring against the per-spawn uniqueId, not against the role/target slot. So the slot-encoding change from `<parent>--subagent--<archetype>--<unique>` to `<parent>--subagent--<targetId>--<unique>` (locked design, #562) is transparent to the helper. No assertions on the role/target slot existed in the migrated test surface beyond what already lives in SubAgentSpawnModeRoutingTests.cs (added in step 3). Validation * Full Gateway.Tests suite: 1865 passed / 1 skipped / 0 failed / 0 warnings under TreatWarningsAsErrors=true. Same total as step 3 (1865) — net behaviour preserved by the typed Mode path; no silent regression in any of the 10 migrated tests. * Full solution build clean (0 W / 0 E). After this step the `case null:` back-compat arm in DefaultSubAgentManager.SpawnAsync has no remaining test callers — it will be deleted in step 5 along with the legacy fields themselves. Refs #562 * feat(gateway): delete legacy SubAgentSpawnRequest fields; Mode is required (#562 step 5/5) Final destructive step of the SubAgentSpawnMode discriminated-union work. The bag of optional top-level fields on SubAgentSpawnRequest has been deleted; every spawn must now pick a Mode (Embody | Mirror) at construction time. The mode-mixing class of bugs (callers populating an inconsistent subset of legacy fields that the runtime silently interpreted) is closed structurally — at the type, at the manager, and at the architecture fence. What changed - SubAgentSpawnRequest: delete Name, ModelOverride, ApiProviderOverride, ToolIds, SystemPromptOverride, Archetype, TargetAgentId. Mode is now `required SubAgentSpawnMode`. Kept: ParentAgentId, ParentSessionId, Task, MaxTurns, TimeoutSeconds, SpawnDepth, ParentToolDenyList, InheritedConversationId, Mode. - SubAgentSpawnTool: stop populating the deleted fields on the constructed request; build the typed Mode (Embody with customisations or Mirror with target) and hand it off. - DefaultSubAgentManager.SpawnAsync: delete the legacy `case null:` arm of the switch — Mode being required makes it structurally unreachable. The default arm is retained for the "unknown future subclass of SubAgentSpawnMode" guard. Deny-list check moved to run AFTER the switch so it reads the typed `toolIds` local resolved from the active arm rather than a now-deleted top-level field; check stays mode-agnostic so a future Mirror that produces a tool list is still policed. Tests - SubAgentModelsTests: required-props list now includes Mode; defaults test constructs with `Mode = new Embody(SubAgentArchetype.General)` and asserts the Mode round-trips; record-equality test uses an Embody mode in place of the deleted Name field. - Migrated remaining test factory helpers to set Mode: SubAgentToolInheritanceTests, DefaultSubAgentManagerActivityTests, DefaultSubAgentManagerTests (+ scaffold InterfaceBackedSubAgentManager reads name/model from Embody customisations), SubAgentCompletionWakeUpTests, SubAgentCompletionWakeDeliveryTests, SubAgentIntegrationTests, SubAgentSpawnDepthTests. - SubAgentToolTests: assertions rewritten from `captured.ModelOverride` / `.ToolIds` / `.SystemPromptOverride` / `.Archetype` to `captured.Mode` pattern-matched as Embody with the customisations and role verified. New architecture fence - tests/architecture/BotNexus.Architecture.Tests/SubAgentSpawnRequestShapeTests.cs: - SubAgentSpawnRequest_DoesNotExpose_LegacyFields — reflection-asserts each of the seven deleted properties is absent. - SubAgentSpawnRequest_Mode_IsRequired_AndTypedAsSubAgentSpawnMode — asserts Mode exists, is typed SubAgentSpawnMode, carries RequiredMemberAttribute, and is non-nullable per NullabilityInfoContext. - DefaultSubAgentManager_PatternMatchesOnMode_NotLegacyFields — source-scan asserts the manager pattern-matches on `request.Mode` and does not read any deleted legacy field via `request.<Name>`. - Fence_DetectsSyntheticLegacyFieldRead — vacuity guard: the legacy-field scan must catch a synthetic violation so a broken regex cannot silently neuter the fence. Validation - dotnet build BotNexus.slnx --nologo --tl:off → clean (0 warnings, 0 errors under TreatWarningsAsErrors). - dotnet test tests/gateway/BotNexus.Gateway.Tests → 1865 passed / 0 failed / 1 skipped (unchanged from steps 3+4). - dotnet test tests/architecture/BotNexus.Architecture.Tests → 53 passed / 0 failed (baseline 49 + 4 new shape pins). Refs #562 * fix(gateway): fold critique-sweep findings into SubAgentSpawnMode DU (#562) Multi-model critique sweep on the step-5 destructive-cleanup commit (36450e4) surfaced two HIGH and two MEDIUM findings. This commit folds the actionable subset into source + tests; deferred items are documented inline. Source fixes ------------ 1. Null-Mode runtime guard (bug-hunt HIGH). `required` is a compile-time guarantee only. A caller using `null!` or a JSON deserializer quirk could surface a runtime null Mode. The default switch arm would dereference `request.Mode.GetType()` and raise NullReferenceException — which propagates as HTTP 500 with an unhelpful stack trace. Added an `ArgumentNullException.ThrowIfNull` on `request.Mode` immediately after the existing request null-check so callers see a clean, named exception with the parameter name. 2. Mirror model fallback bug (bug-hunt MEDIUM). `SubAgentInfo.Model` was computed as `modelOverride ?? configuredDefaultModel ?? parentDescriptor.ModelId` on both Embody and Mirror paths. For Mirror with no modelOverride, this reported the *parent's* model id instead of the *mirrored target's* — silent metadata corruption that downstream telemetry, cost accounting, and observability would compute against the wrong model. Changed to `baseDescriptor.ModelId`, which covers both arms correctly (Embody: baseDescriptor = parentDescriptor; Mirror: baseDescriptor = mirrored target). Test fixes ---------- 3. Architecture fence regex tightening (plan-vs-impl HIGH). The step-5 fence on `DefaultSubAgentManager.cs` used literal `source.Contains(...)` to detect the Mode pattern-match. Mutations like `switch(request.Mode)` (no space) or `case Embody embody` would silently slip past, downgrading the fence to a no-op while leaving the suite green. Replaced with regexes: switch\s*\(\s*request\.Mode\s*\) \b(is|case)\s+Embody\b \b(is|case)\s+Mirror\b Tolerates IDE / formatter whitespace variations and both `case` and `is` pattern shapes. 4. Vacuity guard for the switch-detection clause (plan-vs-impl HIGH). The step-5 vacuity test (`Fence_DetectsSyntheticLegacyFieldRead`) only covered the legacy-field-read clause. Added a sibling `Fence_DetectsSyntheticModeSwitchShapes` that pins the regex behaviour against four synthetic mutations (no-space, padded parens, `is Embody`, `case Mirror`) and against a clean shape (must not false-positive). Without this, a regex regression could silently downgrade the fence while the legacy-field scan continues to pass. 5. JSON null-discriminator test pin (plan-vs-impl MEDIUM). Existing tests pinned missing discriminator (NotSupportedException) and unknown discriminator (JsonException). The `{"mode": null}` shape was not pinned. Added `Mode_JsonDeserialize_NullDiscriminator_Throws` asserting JsonException. A future STJ behaviour change for null discriminators would now be caught immediately. 6. Mirror-model assertion fold-in to routing test. `SpawnAsync_WithModeMirror_UsesTargetDescriptor_AndChildIdUsesTargetSlot` already pinned the *registered descriptor's* ModelId but not the *returned* `info.Model`. Added the assertion so a regression to the parent's model id (the bug fixed in #2 above) fails this test directly rather than slipping through. 7. Null-Mode test pin (bug-hunt HIGH). Added `SpawnAsync_WithNullMode_ThrowsArgumentNullException` to pin the new runtime guard at the manager boundary. Deferred items -------------- * Security HIGH (Mirror authz gap): a caller with `spawn_subagent` can name any registered targetAgentId and inherit that agent's tools / model / system-prompt without authz. Authorization is out of scope for this plan (operator owns auth; only agent-provider auth is in scope). Operators control Mirror target registration — treat the registry as a trust boundary. * Plan-vs-impl HIGH #1: archetype context-table mismatch (my notes said "Tester", code says "Writer"). Code is correct; documentation fix only, not a behavioural defect. * Plan-vs-impl LOW #6/#7: out-of-scope SubAgentInfo contract changes for Mirror's `Archetype = General` label. * Plan-vs-impl MEDIUM #4: Mirror inherits target's tools that may be on the parent's deny-list (intentional pass-through; deny-list is mode-agnostic by design — Mirror is strict pass-through, target registration is the trust boundary). Validation ---------- * Build: 0 warnings / 0 errors under TreatWarningsAsErrors. * Gateway.Tests: 1867 passed / 1 skipped / 0 failed (+2 from step 5). * Architecture.Tests: 54 passed / 0 failed (+1 vacuity guard from step 5's 53). Refs #562. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The security scan flags three files as containing private key material — all false positives:
.github/workflows/security-scan.yml— contains the grep pattern itself.copilot/skills/secret-handling/SKILL.md— documentation examples of key headers.squad/templates/skills/secret-handling/SKILL.md— same (template copy)Fix
.copilot/and.squad/secret-handling paths from the file list before greppingAll three false positives are now excluded.