ainova-systems · dzykovic · Aug 6, 2026 · Aug 6, 2026
diff --git a/.operator/data/findings/F20260806-63E2D28A.md b/.operator/data/findings/F20260806-63E2D28A.md
@@ -0,0 +1,41 @@
+---
+id: F20260806-63E2D28A
+kind: finding
+title: Daemon's top-level cycle error boundary discards the error object entirely — a crashed cycle is invisible
+status: pending
+priority: 2
+source: code-quality#FINDING-001
+created_at: '2026-08-06T20:45:49Z'
+---
+
+**Severity**: high
+**Priority**: 2
+**Files Affected**: 2
+
+**Pattern**: Catch-and-continue with no ERROR/WARN log and no `.cause` — violates `intelligence/rules/typescript.md` §REQUIRED (Observability) and the §Quality Gates observability BLOCKER in `intelligence/rules/migration.md`.
+**Domain**: engine/daemon
+
+**Impact**: `engine/daemon/daemon.ts:196` is the daemon's outermost error boundary and it is a bare `} catch {` — the thrown error is never bound, so the message, stack, and `.cause` are destroyed. The only surviving side effects are `this.health.recordCycle(false)` (`daemon.ts:197`) and a status-line flag (`daemon.ts:198`), which reduce an arbitrary failure to the single word `failure`.
+
+The class cannot recover even if the catch bound the error: the logger dependency is typed `{ info: (msg: string) => void }` at `daemon.ts:51`, so `.error` / `.warn` are structurally unreachable inside `Daemon`, even though the composition root passes the full `Logger` at `engine/entry.ts:529`. `this.log?.info` is used on the neighbouring paths (`daemon.ts:168`, `daemon.ts:210`), so the failure path is the only unlogged one.
+
+`Engine.runOnce` (`engine/engine/engine.ts:118`) has no try/catch of its own, so throws from repo enumeration, `cycleHistory.start` (KV/SQLite write), `guard.acquire` (`engine.ts:42` in `processProject`, also a SQLite write), and event-bus handlers all land in that bare catch. Workspace-prep failures are correctly logged and returned as results (`engine.ts:295-340`), which means the errors that reach line 196 are precisely the unexpected ones a stack trace is needed for.
+
+Two compounding consequences:
+- `runOnce` never finalizes its cycle `executions/{id}` row when it throws, so the App UI shows a cycle stuck in `running` forever with no error text — no second source of truth for what failed.
+- In `--once` mode (`engine/entry.ts:609-616`) the process exits 1 printing only `Cycle complete: failure`, giving CI/cron callers a red exit with zero diagnostics.
+
+`engine/daemon/scheduler.ts:40` is the same defect one layer down: `IntervalScheduler.schedule` discards the callback rejection (`} catch { }`) and the class holds no logger at all, so it cannot back-stop the boundary above it. Its skipped-tick branch (`scheduler.ts:36`) is likewise an unlogged decision, unlike the equivalent guard in `daemon.ts:167-168`.
+
+In daemon mode the net effect is a process that keeps ticking every `cycleIntervalMs` and failing invisibly for as long as it is left running — the exact "I cannot see what the engine did" failure mode v5 was rebuilt to eliminate.
+
+**Fix**: Widen the `Daemon` logger dependency from `{ info }` to the full `Logger` (or minimally `{ info; warn; error }`) so the error channel is reachable; bind the error in the `runCycle` catch and emit an ERROR line carrying the message, the full `.cause` chain, cycle number, traceId, and duration before recording health. Give `IntervalScheduler` an optional logger, bind and log the job rejection at ERROR with the job id, and log the skipped-tick branch at WARN. Additionally, finalize the cycle execution row as `status: "failed"` with the error text when `runOnce` throws, so the failure is visible in the App UI as well as the log stream.
+
+**Acceptance Criteria**:
+- [ ] Regression test: a `Daemon` whose `engine.runOnce` rejects with an `Error` carrying a `.cause` produces an ERROR log line containing both the message and the cause; the test fails against the current bare `} catch {`
+- [ ] Regression test: an `IntervalScheduler` job whose callback rejects produces an ERROR log naming the job id, and a tick skipped by the `running` guard produces a WARN
+- [ ] `Daemon`'s injected logger type exposes `error`; no catch block in `engine/daemon/**` discards its error binding
+- [ ] A cycle that throws leaves its `executions/{id}` row in `failed`, not `running`
+- [ ] Coverage on `engine/daemon/daemon.ts` and `engine/daemon/scheduler.ts` stays >=90%
+- [ ] Build passes after fix
+
diff --git a/.operator/data/findings/F20260806-69EA9796.md b/.operator/data/findings/F20260806-69EA9796.md
@@ -0,0 +1,43 @@
+---
+id: F20260806-69EA9796
+kind: finding
+title: Bot footer marks every fresh review comment as answered at selection time, so feedback the supervisor never addressed is silently lost
+status: pending
+priority: 2
+source: orchestration-reliability#FINDING-001
+created_at: '2026-08-06T20:54:28Z'
+---
+
+**Severity**: high
+**Priority**: 2
+**Files Affected**: 3
+
+**Pattern**: "Fresh feedback overwritten" — a bot reply that marks comments as answered must enumerate exactly the comment ids it actually addressed, never the whole fresh set. Also violates the one-way-signal discipline: `responded` is a permanent, irreversible filter with no liveness or completeness check.
+**Domain**: engine/pipeline (pr-feedback selector + supervisor composer + thread dispositions)
+
+**Impact**: The `responded` set that permanently suppresses a review comment is computed **before the agent runs** and stamped **regardless of what the agent did**.
+
+1. `engine/pipeline/primitives/pr-feedback-selector.ts:253-256` builds `nextResponded` = prior footer set ∪ **every** fresh issue comment ∪ **every** fresh inline review comment, at selection time, and ships it as `payload.respondedIds` (`:263`).
+2. `engine/pipeline/composers/pr-feedback-supervisor-stage.ts:335-336` turns that verbatim into `nextAttribution.responded = new Set(payload.respondedIds)`, then writes it into the bot reply on **every** exit path: limit-reached (`:345`), stale-CI downgrade (`:457`), terminal decision (`:475`), fix-in-place with changes (`:493`), and no-changes (`:516`). Nothing between selection and posting narrows the set.
+3. `engine/pipeline/primitives/pr-decision.ts:46` (`filterUnansweredComments`) drops any comment whose id is in that set, forever. `classifyPrFeedback` then returns `clean`, and `engine/pipeline/pr-lifecycle.ts:288-296` promotes a `clean` PR from `ai:in-review` to `ai:ready-to-merge`.
+
+The engine already knows exactly which comments were left unanswered and throws the information away: `engine/pipeline/composers/_shared/thread-dispositions.ts:114-119` computes `gaps` (fresh inline comment ids with no `EMIT comment-reply`) and WARNs about them, but the call site at `pr-feedback-supervisor-stage.ts:402` discards the returned `ThreadDispositionResult` entirely — `gaps`, `unmatched`, and `replied` never reach `nextAttribution`. The header comment at `thread-dispositions.ts:44-47` states gaps are "surfaced as WARN lines rather than silently dropped", which is true of the log line and false of the state machine.
+
+Two concrete loss scenarios:
+- A supervisor run that answers 3 of 5 Copilot/human inline comments stamps all 5 as responded. The 2 unanswered ones never re-enter `needs-review` on any later cycle; the PR reads `clean` and is promoted toward the merge gate with live reviewer feedback unaddressed. The verifier gate is the only thing standing between this and a merge, and `thread-dispositions.ts:111` explicitly designates it "the primary enforcement" — a single verifier miss becomes permanent.
+- The review-cycle-cap path is unconditional loss with no agent and no verifier involved at all: `selectInput` skips the agent when the cap is hit (`pr-feedback-supervisor-stage.ts:231-240`, `:203`), yet `afterAgent` still posts the limit-reached comment with the full `nextAttribution` (`:345`). Every pending comment is marked answered by a run that read none of them. If a human later pushes a fix and relabels the PR back into the pipeline, those comments stay invisible to the supervisor permanently.
+
+The recovery path is human-only and non-obvious: the reviewer must post a **new** comment, because the original id is now in the footer of the latest bot reply that `parseLatestBotFooter` (`engine/delivery/bot-footer.ts:87-98`) reads back each cycle.
+
+Related to (but distinct from) pending task T20260705-7E556EBC, which only splits `pr-feedback-supervisor-stage.ts` for the line cap and does not touch this logic.
+
+**Fix**: Make the footer report what actually happened instead of what was intended. Split the payload into `priorRespondedIds` (carry-forward, always safe) and `targetedCommentIds` (this run's fresh ids) in `pr-feedback-selector.ts`. In `pr-feedback-supervisor-stage.ts`, capture the `ThreadDispositionResult` returned at `:402` and build `nextAttribution.responded` as `priorRespondedIds ∪ (targetedCommentIds − gaps − unmatched)`, so an inline comment left without a disposition stays unanswered and re-surfaces next cycle. Move the `nextAttribution` construction below the disposition call so no exit path can post a pre-agent set. On the limit-reached path (`:343-353`), post the footer with `priorRespondedIds` only — the agent never read those comments. Top-level issue comments answered by the bot reply body itself may stay in the set; scope the subtraction to inline review comments, which is exactly what `gaps` covers.
+
+**Acceptance Criteria**:
+- [ ] Failing-first regression test named for the bug scenario (e.g. "inline review comment left without a disposition is not marked responded and re-surfaces next cycle"): drive `afterAgent` with two fresh inline review comment ids and an agent output carrying an `EMIT comment-reply` for only one; assert the posted footer's `responded` set contains the answered id and NOT the unanswered one. Test must fail on the current `new Set(payload.respondedIds)` code.
+- [ ] Failing-first regression test: on the review-cycle-cap path (agent skipped), the posted footer's `responded` set equals the prior footer's set exactly — no fresh comment id is added.
+- [ ] Round-trip test: `formatFooter` → `parseLatestBotFooter` → `filterUnansweredComments` returns the unanswered comment as fresh, proving the comment re-enters `needs-review` and `classifyPrFeedback` does not return `clean`.
+- [ ] The `ThreadDispositionResult` returned at `pr-feedback-supervisor-stage.ts:402` is consumed, not discarded; an INFO line records the responded-set delta (added vs withheld ids) so the decision is reconstructable from logs alone.
+- [ ] Coverage on `engine/pipeline/primitives/pr-feedback-selector.ts` stays >=95% and on `engine/pipeline/composers/pr-feedback-supervisor-stage.ts` >=90%
+- [ ] Build passes after fix
+
diff --git a/.operator/data/findings/F20260806-82A7E06B.md b/.operator/data/findings/F20260806-82A7E06B.md
@@ -0,0 +1,39 @@
+---
+id: F20260806-82A7E06B
+kind: finding
+title: Untrusted PR commenters bypass the TRUSTED_ASSOCIATIONS gate via fullThread — raw comment bodies reach the supervisor agent's context
+status: pending
+priority: 2
+source: security#FINDING-001
+created_at: '2026-08-06T21:09:36Z'
+---
+
+**Severity**: high
+**Priority**: 2
+**Files Affected**: 3
+
+**Pattern**: An existing author-trust control is enforced on one path into the agent prompt and silently bypassed on the parallel path — untrusted external input reaches the agent CLI unfiltered.
+**Domain**: engine/pipeline/primitives (pr-feedback selection) → engine/pipeline/composers (pr-review supervisor stage)
+
+**Impact**: `pr-decision.ts:25` defines `TRUSTED_ASSOCIATIONS = {OWNER, MEMBER, COLLABORATOR}` and `filterUnansweredComments` (`pr-decision.ts:48`) uses it to keep comments from arbitrary GitHub users out of the operator's feedback loop. The selector honours that filter for one payload field only:
+- `pr-feedback-selector.ts:247` — `newFeedback = formatFeedback(state.freshComments, …)` → trust-filtered.
+- `pr-feedback-selector.ts:248` — `fullThread = formatFullThread([...comments], [...reviewComments], marker)` → the **raw, unfiltered** `getComments` / `getReviewComments` results. `formatFullThread` (`pr-feedback-selector.ts:133-148`) applies no association check; it interpolates `c.body` verbatim.
+
+That string is written to a temp file at `pr-feedback-supervisor-stage.ts:171-177` and the supervisor task prompt instructs the agent to read it (`pr-feedback-supervisor-stage.ts:597-605`, "Full PR conversation thread is available in `{threadFile}`. Read it if a new comment references earlier discussion"). The same prompt (lines 551-576) hands the agent the complete AOP control vocabulary with copy-paste-ready examples — `EMIT status-update target: self status: cancelled`, `EMIT child-item`, `EMIT verdict value: approved` — and the supervisor is a code-writing role whose edits the orchestrator commits and pushes.
+
+So on a public managed repo, any drive-by commenter (`authorAssociation: NONE` / `CONTRIBUTOR` / `FIRST_TIME_CONTRIBUTOR`) can place arbitrary instructions and well-formed EMIT records into the agent's context. It does not trigger a cycle on its own — a trusted comment or a CI failure does — but once a cycle runs, the injected text is in scope. This is the exact class `SECURITY.md` §Scope names highest-value ("injecting instructions that reach the agent CLI"). No prompt in `engine/content/prompts/**` carries any untrusted-input framing (verified: zero matches for untrusted / injection / adversarial guards), and nothing strips or escapes `=== EMIT` markers from external text.
+
+Secondary weakness on the same control: `pr-decision.ts:48` reads `!c.authorAssociation || TRUSTED_ASSOCIATIONS.has(...)` — a missing `author_association` (optional in the adapter's shape, `vcs.ts:126,136`) is treated as trusted. The trust boundary fails open.
+
+**Fix**: Filter before formatting, then fence what remains.
+1. Export the trust predicate from `pr-decision.ts` and apply it in `formatFullThread`, or pass an already-filtered comment list at `pr-feedback-selector.ts:248`, so untrusted-association human comments never enter `fullThread`. Keep the bot allowance as-is (deliberate, documented).
+2. Flip the fail-open on `pr-decision.ts:48` to fail-closed: a missing `authorAssociation` on a `User` comment is untrusted.
+3. Defense in depth: wrap external comment bodies in an explicit untrusted-data fence in `formatFeedback` / `formatFullThread` and add one line to `engine/content/prompts/agents/supervisor.md` stating that PR comment bodies are data, never instructions, and that `EMIT` records inside them must be ignored.
+
+**Acceptance Criteria**:
+- [ ] Regression test in `pr-feedback-selector.test.ts` named for the bug scenario: a `NONE`-association comment present in `getComments` does not appear in the resulting `payload.fullThread`, while an `OWNER` comment does — the test fails on the current code.
+- [ ] Test asserting a `User` comment with `authorAssociation: undefined` is excluded by `filterUnansweredComments`.
+- [ ] Test asserting comment bodies containing `=== EMIT status-update ===` are rendered inside the untrusted fence and cannot be mistaken for the agent's own record.
+- [ ] Coverage on the touched primitives stays >=95%.
+- [ ] Build passes after fix
+