fix(agent): resolve a forced skill by the same rule as a forced snippet

[CybotTM]

Two issues in the same three functions. #781 is a forced skill and a forced snippet answering the same question differently; #777 is the same forced skill set being ordered two ways.

What the issues got wrong, and what the code says

#781 described the disagreement as living inside AgentRunRequestCodec. It is wider: three places rebuild a forced skill set from persisted uids, and the playground has the same disagreement as the codec.

AgentRunRequestCodec::skillsByUids() also carried a docblock saying the missing enabled filter was deliberate — "forcing a skill overrides its global toggle, the same semantics the playground's force-inject control has". That reads like an intended rule, so it was checked rather than taken: no record decides it, and the playground does not behave that way — availableSkills() lists enabled skills only, so a disabled skill can reach the forced set only from a stale form or a hand-built body. The sentence described neither an intent nor the thing it claimed to copy.

The decision comes from ADR-166, not from me

ADR-166 already wrote the rule down twice. findByUids() is "the lookup for a prompt being assembled now, and a snippet an operator switched off must not enter one", and "a fresh run that forces it gets it through the active-only lookup like any other".

Its "the skill half was already correct" is true of the path it was judging — the resume — and says nothing about the two that compose. So the fix direction is not a coin toss between #781's two options; the record picks one, and this change applies it to the path ADR-166 never looked at. ADR-175 records that and ADR-166 gains the matching :Amended:.

Path	Composing or re-gating	Lookup
ToolPlaygroundController::resolveForcedSkills()	composing now	findByUids() — enabled only
AgentRunRequestCodec::skillsByUids()	composing now (dequeue)	findByUids() — enabled only
ToolLoopService::augmentationFrom()	re-gating sent text	findExistingByUids() — existence

The ordering half (#777)

The resume path iterated findAll() and so returned $defaultOrderings, name ASC; the two composition paths returned the persisted uid order. InputContextClassification::withStricter() keeps the later source on an equal data class, and that source's name is what the refusal message and the governance row carry — so one run blamed one skill before it suspended and another after it resumed. Same ceiling, same outcome, different name in the audit. All three now use the caller's order.

Verification

Ten functional cases against real rows. The fixture is its own and its names sort against its uids — 21 is "Zulu", 22 is "Alpha" — so an order assertion distinguishes the caller's order from name ASC instead of passing under both.

Each assertion was seen to fail before being trusted:

Control	Result
drop the enabled clause from orderedByUid()	findByUidsSkipsDisabledSkills fails
return query order instead of the caller's	3 tests fail, including both findExisting… order cases

Restored, green, git status clean.

One existing unit test broke, and that was a real signal rather than noise. ToolLoopServiceTest::resumeRebuildsTheForcedSkillsToo stubbed findAll(). Its assertion — a disabled skill still resolves on resume — is exactly what must not regress, so the double was moved to the new lookup rather than the test being relaxed. It now also asserts which uids reach that lookup, so the seam is covered here while the filtering is proven against the database, where a stub cannot flatter it.

Gates: phpstan level 10 clean, unit 7137 pass, fuzzy pass, functional -d sqlite on the touched classes 38 pass, cgl and rector -n both stable at PHP 8.2 after applying.

Deliberately not in this PR

A forced source that disappears before a run starts is silent — now uniformly, where before it was silent for snippets and absent for skills. #781 asked for it to be visible. That needs somewhere to show it and a decision on whether a queued run should refuse instead, so it is filed as #809 rather than guessed at here.

The ADR is in this PR rather than ahead of it because it amends a decision instead of taking one.

Closes #781, closes #777

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

[codecov]

Codecov Report

❌ Patch coverage is 10.71429% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.23%. Comparing base (4d360cd) to head (c3f3aac).

Files with missing lines	Patch %	Lines
Classes/Domain/Repository/SkillRepository.php	0.00%	23 Missing ⚠️
...es/Controller/Backend/ToolPlaygroundController.php	0.00%	1 Missing ⚠️
Classes/Service/Agent/AgentRunRequestCodec.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #810      +/-   ##
============================================
- Coverage     87.25%   87.23%   -0.03%     
- Complexity     9761     9770       +9     
============================================
  Files           571      571              
  Lines         31597    31599       +2     
============================================
- Hits          27570    27565       -5     
- Misses         4027     4034       +7     

Flag	Coverage Δ
unit	65.31% <10.71%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
Classes/Service/Tool/ToolLoopService.php	97.31% <100.00%> (-0.02%)	⬇️
...es/Controller/Backend/ToolPlaygroundController.php	73.28% <0.00%> (+1.18%)	⬆️
Classes/Service/Agent/AgentRunRequestCodec.php	95.45% <0.00%> (+7.95%)	⬆️
Classes/Domain/Repository/SkillRepository.php	40.81% <0.00%> (-36.11%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[CybotTM]

Rebased onto main after #801 merged, now c3f3aac0. Seven siblings landed while this waited, so both conflicting files were rebuilt, not merged — "ours holds only my additions" stops being true from the second merge of a series, which is the failure this repository already carried to main once.

Documentation/Adr/Index.rst and CHANGELOG.md were each reset to their main version verbatim and this branch's content re-inserted: the toctree line in ascending position, the two changelog blocks under their existing ### Changed and ### Fixed headings.

Verified after rather than assumed:

• toctree reads … Adr172, Adr173, Adr174, Adr175 and every Adr*.rst on disk is listed;
• three ### headings in [Unreleased], 16 bullets where main's 14 plus this branch's 2 is expected, each of the two blocks present exactly once, no bullet appearing twice;
• no real conflict markers — matched as ^<<<<<<< / ^>>>>>>> with the trailing space, since a bare ======= is RST underlining;
• check-changelog-unreleased.php passes.

The review record still applies, and that is checked rather than claimed: the change set is 11 files changed, 414 insertions(+), 60 deletions(-) across the same eleven paths as before the rebase, so nothing in the reviewed code moved.

One thing worth naming, because it was a real risk and not just a formality: #807 merged while this waited, and it added a test that reads the whole ADR corpus and asserts every File.php:NNN citation. ADR-175 is new text in that corpus. I checked before enqueueing #807 that this branch adds no such citation; the rebased unit run now confirms it against the merged tree rather than against my reading — 7200 tests, exit 0.

phpstan level 10 clean; the ten new functional cases pass alongside the two existing ones.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

[CybotTM]

Review record — no Copilot review exists for this diff (monthly, account-wide quota exhausted), so decision=APPROVED comes from github-actions and not from a reviewer who read the change. What was checked:

The issue was wrong about where the defect lives, and the docblock was the trap. #781 placed the disagreement inside AgentRunRequestCodec. There are three copies of the lookup and the playground has the same one. The codec then carried a docblock calling the missing enabled filter deliberate — "forcing a skill overrides its global toggle, the same semantics the playground's force-inject control has". That reads like a decided rule, so it was checked instead of believed: no record decides it, and the playground does not do it — availableSkills() lists enabled skills only.

The direction is ADR-166's, not mine. It writes the rule twice — findByUids() is "the lookup for a prompt being assembled now", and "a fresh run that forces it gets it through the active-only lookup like any other". Its "the skill half was already correct" is scoped to the resume path it was judging. A queued run has composed nothing at enqueue, so dequeuing it is that assembly. ADR-175 records this and ADR-166 gains the matching :Amended:.

Both new assertions were seen to fail. Dropping the enabled clause fails one test; returning query order instead of the caller's fails three. Restored, green, tree clean.

An existing unit test broke and was treated as a signal, not noise. It stubbed findAll(); its assertion — a disabled skill still resolves on resume — is exactly what must not regress. The double moved to the new lookup rather than the test being relaxed, and it now also asserts which uids reach it, so the seam is covered in the unit test while the filtering is proven against the database.

Post-rebase: change set byte-identical at 11 files changed, 414 insertions(+), 60 deletions(-); unit 7200 pass on the merged tree, which also exercises #807's corpus test against the new ADR-175; phpstan level 10 clean; cgl and rector -n stable at PHP 8.2.

Not in this PR, deliberately: a forced source dropped before a run starts is silent — now uniformly so. That is #809, split out because making it visible needs a place to show it and a decision on whether a queued run should refuse instead.

Merging on that basis.