# Residual review findings — feat/u11-derive-agent-skills (HEAD 36358c0)

Generated by `/compound-engineering:lfg` autofix code-review run `20260425-120817-5b754f8f` on 2026-04-25. Persisted as a fallback because no PR existed at handoff time. The PR opened by `ce-commit-push-pr` should pull these into its description; this file is the durable record either way.

## Run context

- **Mode:** autofix
- **Plan:** `docs/plans/2026-04-25-005-feat-u11-derive-agent-skills-plan.md`
- **Run artifact:** `.context/compound-engineering/ce-code-review/20260425-120817-5b754f8f/`
- **Reviewers dispatched:** correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, security, reliability, adversarial (9/9 returned)
- **Verdict:** Ready with fixes
- **Safe-auto fixes applied:** 6 (commit `36358c0`)

## Residual Review Findings

### P1 (high-impact, manual handoff)

- **[P1] packages/api/src/lib/derive-agent-skills.ts:119 — Concurrent-write race against existing-slugs SELECT outside the transaction.** Two parallel `AGENTS.md` PUTs on the same agent, or a `setAgentSkills` mutation in flight while an `AGENTS.md` PUT runs derive, can produce non-deterministic `agent_skills` set. The existing-slugs `SELECT` happens outside `db.transaction`, so the read-modify-write is non-atomic. Reviewers: reliability + adversarial (cross-reviewer corroboration → conf 100). Fix direction: move `existingRows` SELECT inside `db.transaction()` with `SELECT ... FOR UPDATE` on the agent row, or accept best-effort + add a periodic reconciler. Plan §008 U13 (DynamoDB advisory lock) is the long-term fix.
- **[P1] packages/api/src/lib/derive-agent-skills.ts:78 — `composeList(includeContent=true)` does N×3 sequential S3 GETs per agent.** A 5-folder agent ≈ 165 sequential S3 GETs; likely exceeds the plan's stated <500 ms budget at enterprise scale (4 enterprises × 100+ agents × ~5 templates). Reviewer: reliability (conf 75). Fix direction: parallelize per-path composes inside `composeList` with bounded `Promise.all`, or add an AGENTS.md-only filter so we only fetch the routing-bearing files.
- **[P1] packages/api/src/lib/derive-agent-skills.ts:156 — Resource exhaustion: massive AGENTS.md (~30K+ slugs) crashes derive at the Postgres parameter limit, leaving the DB perpetually stuck.** Reviewer: adversarial (conf 75). Fix direction: batch INSERT/DELETE in chunks ≤1000 rows or cap derived-set size + reject oversized AGENTS.md at the handler.
- **[P1] packages/api/src/lib/derive-agent-skills.ts:155 — `deriveAgentSkills` bypasses the template-ceiling check that `setAgentSkills` enforces (`packages/api/src/lib/skills/permissions-subset.ts`).** A tenant admin authoring AGENTS.md can introduce slugs outside the agent's `template.skills` ceiling — `setAgentSkills` would reject this, derive accepts it. Reviewer: security (conf 75). Fix direction: load `agent.template_id` + `skill_catalog` and gate derived slugs against the template ceiling, OR document the deliberate trust delegation to the AGENTS.md write-authz path.

### P2 (moderate, manual handoff)

- **[P2] packages/api/workspace-files.ts:472 — Template/defaults AGENTS.md edits silently desync `agent_skills` until each downstream agent rewrites.** Reviewer: reliability (conf 75). The current `handlePut` template/defaults branch only invalidates the templateScope cache and does NOT fan out derive to inheriting agents. `chat-agent-invoke` reads from `agent_skills` (not the composed tree), so the inherited-template AGENTS.md change won't surface until each downstream agent's next `AGENTS.md` write triggers derive. Fix direction: on template/defaults AGENTS.md put, fan out derives to downstream agents via SQS, or document a periodic reconciler.

## Pre-existing concerns flagged but not introduced by this PR

- **[P0 / pre-existing] packages/api/workspace-files.ts:718 — apikey PUT to AGENTS.md lacks target-agent binding.** Cross-agent privilege risk within a tenant. Reviewer: security. Should be tracked separately from U11.
- **[P2 / pre-existing] packages/api/workspace-files.ts:114 — `S3Client` has no explicit request/connect timeout.** SDK v3 defaults have no socket timeout. Reviewer: reliability.
- **[P2 / pre-existing] packages/api/workspace-files.ts:421 — `regenerateManifest` failure between S3 put and derive returns generic 500 with no retry guidance.** Reviewer: reliability.

## Advisory (report-only — no follow-up unless someone picks them up)

- Maintainability: `workspace-map-generator` (DB→file) and `derive-agent-skills` (file→DB) walk skill rows in opposite directions and may drift. Acceptable transition cost; U21 should explicitly delete the legacy parser.
- Security P2: derive hard-deletes `agent_skills` rows on slug disappearance, dropping `permissions`/`config` metadata silently. Suggest forensic-trail logging before delete.
- Adversarial P3: lowercase / trailing-whitespace `AGENTS.md` path variants succeed at S3 but skip derive (silent drift).
- Adversarial P2: derive-inserted rows have NULL `permissions`/`config`/`rate_limit_rpm`/`model_override` — verify `chat-agent-invoke`'s `skills_config` build tolerates NULL.
- Reliability P3: deprecation `console.warn` on every `setAgentSkills` call has no rate limiting (acceptable for U17–U21 transition window).
- Agent-native P2: no Strands tool to write `AGENTS.md` (`write_memory` is path-locked to `memory/`); no `list_workspace_files` enumeration tool. Track as follow-up before U21 retires admin-UI authoring.
- Agent-native P3: handler returns `{ ok: true, deriveWarnings? }` only on warning — proposes always returning `{ derive: { changed, addedSlugs, removedSlugs, agentsMdPathsScanned, warnings } }` so admin SPA + future `write_agents_md` tool see the effect.

## Past solutions cited

- `docs/solutions/best-practices/every-admin-mutation-requires-requiretenantadmin-2026-04-22.md` — confirmed U11 deprecation `console.warn` placed correctly AFTER `requireTenantAdmin`.
- `docs/solutions/logic-errors/compile-continuation-dedupe-bucket-2026-04-20.md` — addressed via `DeriveResult.changed` JSDoc tightening (autofix #3).
- `docs/solutions/workflow-issues/skill-catalog-slug-collision-execution-mode-transitions-2026-04-21.md` — slug-disappearance hard-delete loses metadata; tracked as Security P2 advisory.
