diff --git a/.operator/data/findings/F20260806-63E2D28A.md b/.operator/data/findings/F20260806-63E2D28A.md new file mode 100644 index 0000000..c859699 --- /dev/null +++ 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 new file mode 100644 index 0000000..b0f8a58 --- /dev/null +++ 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 new file mode 100644 index 0000000..cb123c2 --- /dev/null +++ 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 + diff --git a/.operator/data/findings/F20260806-9B0ADAA5.md b/.operator/data/findings/F20260806-9B0ADAA5.md new file mode 100644 index 0000000..a20f616 --- /dev/null +++ b/.operator/data/findings/F20260806-9B0ADAA5.md @@ -0,0 +1,34 @@ +--- +id: F20260806-9B0ADAA5 +kind: finding +title: 'The stage-logic → composers rename was never propagated: the D-503 ESLint guard, the boundary rule, and the architecture doc all target a directory that no longer exists' +status: pending +priority: 2 +source: consistency#FINDING-001 +created_at: '2026-08-06T20:49:49Z' +--- + +**Severity**: high +**Priority**: 2 +**Files Affected**: 8 + +**Pattern**: A directory rename (`engine/pipeline/stage-logic/` → `engine/pipeline/composers/`) landed in code without updating its single-source-of-truth rule, its enforcement gate, or the canonical architecture doc — violating `intelligence/rules/dev-context-engineering.md` ("a convention change updates its rule in the same commit as the code that changes it"; "documentation that drifts from code is a defect") and leaving a CI gate that provably matches no files. +**Domain**: engine/pipeline (composers layer) + eslint.operator.config.js + intelligence/rules + docs + +**Impact**: `engine/pipeline/stage-logic/` does not exist — `ls engine/pipeline/` returns `cleanup.ts`, `composers/`, `generic-stage.ts`, `generic-stage-vars.ts`, `primitives/`, `pr-lifecycle.ts`, `run-stage.ts`, `types.ts`. The stage-logic layer is now `engine/pipeline/composers/` (6 stage files + `_shared/` with 3 modules). Four surfaces still name the dead path: + +1. **Dead CI gate.** `eslint.operator.config.js:78` scopes the D-503 boundary guard to `files: ["engine/pipeline/stage-logic/**/*.ts", "engine/pipeline/run-stage.ts"]`. The first glob matches nothing, so the two `no-restricted-syntax` selectors at `:83` and `:88` — which forbid `vcs.getCheckRuns` and `deps.vcs.getCheckRuns` in stage code — are unenforced across the entire composer layer. `grep -c composers eslint.operator.config.js` returns `0`. The composers are exactly the files that reach for `deps.vcs.*` today (`closed-pr-recovery.ts:159` `deps.vcs.getComments`, `:213` `vcs.getCodeReviews`), so a composer adding `deps.vcs.getCheckRuns` would pass `npm run lint` while bypassing the `observeChecks` / `writeChecksContextFile` primitives the rule exists to protect. The comment at `:70-76` states the rule's purpose is to "prevent future contributors from quietly bypassing the architecture" — it currently cannot. +2. **Boundary rule names the wrong directory.** `intelligence/rules/migration.md:84` (Quality Gates): "Only `engine/pipeline/primitives/**` and `engine/pipeline/stage-logic/**` may call `git.*` / `PRManager.*` / `VCSPlatform.*` / `AgentRuntime.*`." `composers/**` is nowhere in that rule, yet it makes exactly those calls (`aop-planner-stage.ts:154` `deps.prManager.markProcessing`, `pr-feedback-supervisor-stage.ts:124` `git.commitCount`, `:187` `deps.git.headSha`, `discovery-iteration-stage.ts:192` `deps.agentRuntime.run`). Since these rules are synced into `AGENTS.md` / `.claude` / `.cursor` and loaded into every agent run, a reviewer or agent applying the rule verbatim reads the whole composer layer as a P1 boundary violation and may "fix" correct code. +3. **Canonical doc describes files that do not exist.** `docs/architecture-v5.md` opens at `:5` with "Every claim in this doc is the code's current shape — drift between this doc and `engine/` is a bug", then at `:45-58` renders a layout tree with `stage-logic/` holding `supervisor.ts`, `finding-plan.ts`, `task-execute.ts`, `research.ts`, `retrospective.ts`, `rejection-handler.ts`, `errors.ts`, `_shared/scratch.ts` — none of which exist. §1.2 at `:176` says "`engine/pipeline/stage-logic/` currently holds 6 stage-named TypeScript files". `composers` appears zero times in `docs/`, so the doc has no description of the layer that actually exists. +4. **Second stale path in the same gate.** `eslint.operator.config.js:53` tells violators to use `agents/workflow/stages.yaml`; neither `agents/` nor `engine/agents/workflow/` exists — the real path is `engine/content/prompts/stages.yaml` per `intelligence/rules/context.md`. The same dead path is repeated in `intelligence/rules/typescript.md:32` and `:63`, `docs/workflow.md:162` and `:618`, `intelligence/agents/operator-ts-developer.md:44`, and `intelligence/skills/operator-migrate-next/SKILL.md:55`. `.operator/context/project.md:76` lists `stage-logic/**` among High Priority Areas. + +**Fix**: Repoint every surface at the paths that exist, in one change. Widen the D-503 glob at `eslint.operator.config.js:78` to `["engine/pipeline/composers/**/*.ts", "engine/pipeline/run-stage.ts"]` (keep `run-stage.ts`) and update its message text from "Stage-logic" to the composer layer; correct the `stages.yaml` path in the `:53` message to `engine/content/prompts/stages.yaml`. Rewrite `intelligence/rules/migration.md:84` to grant the primitive-call exemption to `engine/pipeline/composers/**`. Replace the `stage-logic/` subtree in `docs/architecture-v5.md:45-58` and §1.2 at `:176` with the real `composers/` contents, restating the dissolution plan against the current file names. Fix the `agents/workflow/stages.yaml` occurrences in `intelligence/rules/typescript.md:32,63`, `docs/workflow.md:162,618`, `intelligence/agents/operator-ts-developer.md:44`, and `intelligence/skills/operator-migrate-next/SKILL.md:55`, then re-run `bash intelligence/sync/scripts/sync.sh`. Update `.operator/context/project.md:76`. + +**Acceptance Criteria**: +- [ ] `grep -rn "stage-logic" eslint.operator.config.js intelligence/rules docs .operator/context` returns no path reference to `engine/pipeline/stage-logic/` (prose in `CHANGELOG.md` describing historical releases may stay) +- [ ] `grep -rn "agents/workflow/stages.yaml" docs intelligence eslint.operator.config.js` returns nothing; the only stages.yaml path in rules and lint messages is `engine/content/prompts/stages.yaml` +- [ ] The D-503 `no-restricted-syntax` block names `engine/pipeline/composers/**/*.ts`; adding a temporary `deps.vcs.getCheckRuns(1)` call inside any file under `engine/pipeline/composers/` makes `npm run lint` fail, and removing it makes lint pass again +- [ ] `docs/architecture-v5.md` layout tree and §1.2 list the files actually present under `engine/pipeline/composers/` (6 stage files + `_shared/`), with no reference to `supervisor.ts`, `finding-plan.ts`, `task-execute.ts`, `research.ts`, `retrospective.ts`, or `rejection-handler.ts` +- [ ] `intelligence/rules/migration.md` Quality Gates names `engine/pipeline/composers/**` as the layer permitted to call `git.*` / `PRManager.*` / `VCSPlatform.*` / `AgentRuntime.*` alongside `primitives/**` +- [ ] Build passes after fix + diff --git a/.operator/data/findings/F20260806-9FAA4A77.md b/.operator/data/findings/F20260806-9FAA4A77.md new file mode 100644 index 0000000..6ee4982 --- /dev/null +++ b/.operator/data/findings/F20260806-9FAA4A77.md @@ -0,0 +1,46 @@ +--- +id: F20260806-9FAA4A77 +kind: finding +title: Improver prompt never asks for the week/date/analyzer frontmatter its own output format requires — retrospective validation fails on every successful run +status: pending +priority: 2 +source: prompt-quality#FINDING-001 +created_at: '2026-08-06T20:58:59Z' +--- + +**Severity**: high +**Priority**: 2 +**Files Affected**: 4 + +**Pattern**: Prompt ↔ code drift — a role prompt's mandated output shape contradicts the parser contract, the reviewer criteria, and the format template that the engine enforces for that role. +**Domain**: engine/content/prompts/agents (improver / retrospective stage) + +**Impact**: The retrospective stage (`stages.yaml` → `composer: weekly-metrics`, `agentRole: improver`) validates the improver's stdout through `parseAgentOutput(stripped, ROLE_OUTPUT_FORMATS["improver"])` at `engine/pipeline/composers/weekly-metrics-stage.ts:286`. That resolves to format `improver`, whose required frontmatter fields are `["week", "date", "analyzer"]` (`engine/agents/output-parser.ts:16`); `requiresFrontmatter` is therefore true and line 152-153 throws `improver output requires YAML frontmatter but none found` whenever the report has no `---` block. + +The shipped prompt guarantees exactly that failure. `engine/content/prompts/agents/improver.md:129-144` specifies the entire output as a fenced markdown report beginning `## Optimization {WEEK}` with `### Prompt Changes Applied` / `### Task Changes` / `### Status Reconciliation` / `### New Patterns (watching)` / `### No Changes Needed` — no frontmatter anywhere. Worse, lines 76-84 ("Boundary: frontmatter belongs to the orchestrator … You never directly create, update, or delete YAML frontmatter on any work-item file (`.operator/data/retrospectives/*.md`)") actively instruct the agent NOT to emit one. The only frontmatter guidance the improver ever sees is the conditional `context/base.md:9` ("If output format specifies YAML frontmatter `---`, your FIRST character must be `-`") — and improver.md specifies none. + +This check runs on the SUCCESS path: `weekly-metrics-stage.ts:260` already returned for any non-approved verdict, so the throw at line 286 is reached only after the agent succeeded. Every clean weekly retrospective therefore emits the WARN at line 289 (`improver output failed frontmatter validation, persisting cleaned output`) and silently writes an unvalidated `.operator/data/retrospectives/{WEEK}.md`. The format contract is effectively dead code — it can never pass, so it catches nothing. + +Two more sides of the same drift compound it: +- `engine/content/prompts/agents/verifier/improvement.md:14` — "**File completeness**: Retrospective file must be properly formatted with valid frontmatter." The improver has `review: true` in `engine/content/defaults/agents.yaml`, so this criteria file is loaded as `verifier/improvement` (`weekly-metrics-stage.ts:209`) and the reviewer is asked to gate on a property the prompt forbids producing — a review loop that can burn `maxRetries: 2` and fail the weekly learning cycle for a reason the agent was never told to satisfy. +- `engine/content/templates/formats/improver.txt` requires `# Improvement - {WEEK}` plus `## Prompt Changes`, `## Task Queue Changes`, `## New Patterns`, `## Repeated Patterns` — a heading set that matches neither improver.md's `## Optimization {WEEK}` / `### …` headings nor each other. + +Nothing pins this today: `engine/pipeline/composers/weekly-metrics-stage.test.ts:318-352` feeds a fixture whose frontmatter is only `title:` (still missing all three required fields, so it also throws) and a fixture with no frontmatter, and both assertions pass identically through the fallback — the test suite cannot tell the valid path from the broken one. + +**Fix**: The prompt is the wrong side — correct improver.md, not the parser. In `engine/content/prompts/agents/improver.md` §Output, prepend the required frontmatter to the mandated report block: + + --- + week: {WEEK} + date: + analyzer: improver + --- + +and add one sentence clarifying that this frontmatter belongs to the improver's OWN report file, which is a stage artifact and not a work-item file — so it does not contradict the §Boundary rule at lines 76-84 (which governs `findings/*.md`, `tasks/*.md`, and status flips on retrospective work items). Then align the report headings across the three sources of truth so `formats/improver.txt`, `improver.md`, and `verifier/improvement.md` describe one shape rather than three; pick improver.md's richer heading set (it carries `### Status Reconciliation`, which the format template lacks) and update `formats/improver.txt` to match. + +**Acceptance Criteria**: +- [ ] `engine/content/prompts/agents/improver.md` §Output mandates a `---` frontmatter block containing `week`, `date`, and `analyzer` before the report headings +- [ ] A contract test (extend `engine/pipeline/primitives/agent-output-protocol.prompt-contracts.test.ts`, which already reads the shipped prompts via `resolveContentPath`) lifts the Output example fence out of the real `improver.md` and asserts `parseAgentOutput(example, ROLE_OUTPUT_FORMATS.improver)` does NOT throw — this test fails on the pre-fix prompt +- [ ] A regression test in `weekly-metrics-stage.test.ts` asserts the WARN "output failed frontmatter validation" is NOT logged for an approved run whose output matches the improver.md example (the existing two tests cannot distinguish the branches) +- [ ] `engine/content/templates/formats/improver.txt` heading list and `engine/content/prompts/agents/verifier/improvement.md:14` describe the same shape improver.md mandates +- [ ] Build passes after fix + diff --git a/.operator/data/findings/F20260806-CBFE09B0.md b/.operator/data/findings/F20260806-CBFE09B0.md new file mode 100644 index 0000000..2fe6e83 --- /dev/null +++ b/.operator/data/findings/F20260806-CBFE09B0.md @@ -0,0 +1,36 @@ +--- +id: F20260806-CBFE09B0 +kind: finding +title: CRLF frontmatter fix left two consumer parsers unpinned — pr-lifecycle overrides and prompt-source stripping still break on CRLF +status: pending +priority: 2 +source: test-strategy#FINDING-001 +created_at: '2026-08-06T21:16:47Z' +--- + +**Severity**: high +**Priority**: 2 +**Files Affected**: 2 + +**Pattern**: Contract change without cross-boundary pin tests — the CRLF-tolerant frontmatter contract was fixed and pinned in one parser at a time, leaving parallel parsers carrying the pre-fix bug with LF-only fixtures. +**Domain**: engine/pipeline/pr-lifecycle.ts, engine/agents/kv-prompt-source.ts + +**Impact**: This is the third recurrence of the exact failure mode finding `F20260711-EEC50328` named. Task `T20260711-92D6C0BC` (commit `a51e3b6`) fixed only `discovery-selector.ts` and added the repo's only CRLF fixture. Two parsers still use LF-only regexes, verified broken by running them against CRLF input: + +- `engine/pipeline/pr-lifecycle.ts:102` — `content.match(/^---\n([\s\S]*?)\n---/)` returns `null` on CRLF, so `readItemOverrides` returns `{}` and every per-item `lifecycle_*` override is silently discarded. An item carrying `lifecycle_promote_to_ready_after_idle_hours: null` (explicit "never auto-promote") falls back to the system default and gets promoted anyway. This is on the automated PR state-transition path. +- `engine/agents/kv-prompt-source.ts:123` — `stripFrontmatter` fails to match on CRLF, so raw YAML frontmatter is concatenated verbatim into the composed agent prompt. It is applied to the repo-owned user extension layer `{automationDir}/agents/{topic}.md`, a git-checked-out file. Leaking a literal `---\nkey: value\n---` into a prompt is exactly what the AOP parser's `raw-frontmatter-leak` guard exists to reject. + +Both consumers read files from a managed-repo git checkout, so CRLF arrives via `core.autocrlf=true` or a Windows host (the operator host is win32). Neither has a CRLF test: `pr-lifecycle.test.ts:257` builds its fixture with `.join("\n")` and `kv-prompt-source.test.ts:73` uses `"---\nstage: x\n---\n\nREAL BODY"`. Repo-wide, `\r\n` appears in exactly one frontmatter fixture (`discovery-selector.test.ts:131`). + +Four other parsers were checked and are already CRLF-safe: `work-items.ts:302`, `file-backed.ts:122`, `observe-status.ts:59` (`/^---\s*$/m`), and `agents/frontmatter.ts:9,27` plus `output-parser.ts:74`. + +**Fix**: Make both regexes CRLF-tolerant the same way the fixed parsers are — `/^---\r?\n([\s\S]*?)\r?\n---/` in `pr-lifecycle.ts` (and split its captured block on `/\r?\n/`), and `/^---\r?\n[\s\S]*?\r?\n---\r?\n?/` in `kv-prompt-source.ts`. Add a CRLF fixture to each colocated test, written failing-first against the current regex. To stop the fourth recurrence, extract the delimiter pattern into one shared helper the remaining parsers import, or add a lint/test that asserts no `^---\n` literal survives outside that helper. + +**Acceptance Criteria**: +- [ ] `pr-lifecycle.test.ts` adds "applies lifecycle_promote_to_ready_after_idle_hours from CRLF frontmatter (regression)" — same setup as the existing LF test but with the fixture joined by `\r\n`, asserting `promoted === 0` and `skipped === 1` +- [ ] `pr-lifecycle.test.ts` adds "honors a null lifecycle override written with CRLF line endings (regression)" so the explicit opt-out is pinned, not just the numeric override +- [ ] `kv-prompt-source.test.ts` adds "strips CRLF frontmatter from the KV system layer (regression)" asserting `loadChain` output equals `"REAL BODY"` for body `"---\r\nstage: x\r\n---\r\n\r\nREAL BODY"` +- [ ] `kv-prompt-source.test.ts` adds "strips CRLF frontmatter from the file-backed user extension layer (regression)" writing the `.operator/agents/{topic}.md` fixture with `\r\n` into a real temp dir and asserting no `---` survives in the composed prompt +- [ ] Each new test fails on the pre-fix regex and passes after +- [ ] Build passes after fix + diff --git a/.operator/data/findings/F20260806-E4AA533B.md b/.operator/data/findings/F20260806-E4AA533B.md new file mode 100644 index 0000000..27e699e --- /dev/null +++ b/.operator/data/findings/F20260806-E4AA533B.md @@ -0,0 +1,40 @@ +--- +id: F20260806-E4AA533B +kind: finding +title: Retrospective PR-feedback boundary fabricates 'No merged/rejected AI PRs this week' on any GitHub failure, with zero logging +status: pending +priority: 2 +source: resilience-boundaries#FINDING-002 +created_at: '2026-08-06T21:04:11Z' +--- + +**Severity**: high +**Priority**: 2 +**Files Affected**: 2 + +**Pattern**: Bounded self-healing, never silent recovery — a `catch {}` that swallows the original error and substitutes fabricated data; plus an I/O primitive with no `OperationContext` and no logger. +**Domain**: `engine/work-items/work-items.ts` (retrospective feedback collectors) + `engine/pipeline/primitives/metrics-aggregator.ts` + +**Impact**: The retrospective/learning loop is fed a false statement of fact whenever GitHub hiccups, and nothing in the log says so. + +- `engine/work-items/work-items.ts:975-980` — `collectMergedPRFeedback` wraps `vcs.getCodeReviews({ state: "closed" })` in `try { … } catch { return "No merged AI PRs this week"; }`. The caught error is discarded entirely: no `WARN`, no `.cause`, no rethrow. +- `engine/work-items/work-items.ts:993-998` — `collectRejectedPRFeedback` repeats the identical swallow with `"No rejected AI PRs this week"`. +- Both strings land verbatim in the markdown brief at `engine/pipeline/primitives/metrics-aggregator.ts:89-92` (`## Merged PR Feedback` / `## Rejected PR Feedback`), which is handed to the retrospective agent as its `taskContent`. A transient 502 / secondary-rate-limit / expired token therefore makes the agent read, as ground truth, that the operator shipped and had rejected nothing all week — and the retrospective is the stage that spawns agent-improvement work items from exactly that input. +- Reproduction: make `vcs.getCodeReviews` reject once (`throw new Error("502")`). `aggregateRetrospectiveMetrics` returns a brief containing "No merged AI PRs this week" and "No rejected AI PRs this week", and the process emits no log line at any level. Because `GitHubVCS.getCodeReviews` caches per state, one failure poisons both sections in the same run. +- The same boundary is simultaneously under-guarded two lines later: `formatPRFeedback` calls `vcs.getComments(pr.id)` (`work-items.ts:1018`) and `vcs.getReviewComments(pr.id)` (`work-items.ts:1026`) inside a loop with no error handling at all, so an identical transient failure on the *second* API call throws out of the primitive and fails the whole stage. The boundary over-swallows the first call and under-guards the rest — the operator cannot predict which behaviour a GitHub blip produces. +- `MetricsAggregatorDeps` (`metrics-aggregator.ts:18-24`) carries `vcs`, `kindRegistry`, `workspacePath`, `conventions` — no `OperationContext` and no `Logger`. Verified: the file contains zero `log.*` calls and zero `OperationContext` references, so a caller cannot pass a logger even if it wanted to. This is an I/O function (VCS reads + filesystem reads) that completes with no INFO summary. + +**Fix**: Stop fabricating and start reporting. +1. Add an optional `log?: Logger` parameter to `collectMergedPRFeedback` / `collectRejectedPRFeedback` (the module already imports `Logger` and uses the `opts.log?.info(...)` convention at `work-items.ts:835`), and on catch emit `log?.warn` with the operation, the PR state queried, and the full error including `.cause` before degrading. +2. Make the degraded text self-identifying rather than a false claim — e.g. `"PR feedback unavailable — GitHub read failed: "` — so the retrospective agent can tell "quiet week" from "GitHub was down". +3. Wrap the per-PR `getComments` / `getReviewComments` calls in the same catch-and-warn so one bad comment fetch degrades that PR's block instead of failing the stage. +4. Thread `OperationContext` + `Logger` through `MetricsAggregatorDeps` and log one INFO summary at exit (PRs harvested, sections degraded, duration), per the observability rule for every I/O function. + +**Acceptance Criteria**: +- [ ] Regression test: `collectMergedPRFeedback` with a `getCodeReviews` stub that rejects produces a WARN carrying the error, and its returned text does NOT assert "No merged AI PRs this week"; the test fails on the pre-fix code. +- [ ] Regression test: the same for `collectRejectedPRFeedback`. +- [ ] Regression test: a `getComments` rejection on one PR degrades only that PR's block and does not throw out of `aggregateRetrospectiveMetrics`. +- [ ] `aggregateRetrospectiveMetrics` accepts `OperationContext` + `Logger` and emits at least one INFO summary on the success path. +- [ ] Coverage on both touched files stays >=90% (>=95% for `metrics-aggregator.ts` as a primitive). +- [ ] Build passes after fix + diff --git a/.operator/data/findings/F20260806-FA3943B9.md b/.operator/data/findings/F20260806-FA3943B9.md new file mode 100644 index 0000000..a793f1a --- /dev/null +++ b/.operator/data/findings/F20260806-FA3943B9.md @@ -0,0 +1,34 @@ +--- +id: F20260806-FA3943B9 +kind: finding +title: Dead engine-defaults keys — workspace.baseDir and conventions.patterns are seeded, schema-validated, and UI-editable, but no runtime path reads them +status: pending +priority: 4 +source: staleness#FINDING-001 +created_at: '2026-08-06T21:12:15Z' +--- + +**Severity**: medium +**Priority**: 4 +**Files Affected**: 6 + +**Pattern**: Dead configuration — keys parsed, validated, seeded into KV, and exposed in the config editor while consumed by no runtime path (one survives only through a test). +**Domain**: `engine/content/defaults/defaults.yaml` → `packages/core/src/schemas/engine-defaults.schema.ts` → `engine/config/loader.ts` → `engine/infra/env.ts` + +**Impact**: `loadEngineDefaults` (`engine/storage/seed-sources.ts:168-172`) copies the whole `defaults.yaml` document verbatim into `kv:engine-defaults/global`; `engineDefaultsSchema` is registered in the category-schema map (`packages/core/src/schemas/index.ts:150`) and re-validated on every read (`engine/config/loader.ts:83-88`), and the app renders the category in its JSON editor (`app/src/lib/baseline.ts:129`). Three keys in that editable document are inert: + +1. `workspace.baseDir: /home/runner/workspaces` (`defaults.yaml:9-10`, schema `engine-defaults.schema.ts:52-54`). `buildDefaults` (`loader.ts:23-53`) never copies it, `DefaultsConfig` (`packages/core/src/types/config.ts:77-93`) has no `workspace` field, and `entry.ts:78` calls `loadEnv(resolve(args.configDir, ".."))` with no second argument — so `env.ts:52` always resolves `defaultBaseDir === undefined` and falls through to `{operatorDir}/repos`. A repo-wide grep for `baseDir` returns exactly two hits: the yaml line and the schema line. The value is doubly stale: `/home/runner/...` is a GitHub-Actions-runner path from the retired CI-cron model (the same retired model behind `staleness#FINDING-001`), and it contradicts `docs/deployment.md:19`, which correctly documents the default as `$OPERATOR_DIR/repos`. An operator who edits this key in the UI gets a silent no-op. + +2. `loadEnv`'s `defaultWorkspaceBaseDir` parameter (`env.ts:77-87`) is the only consumer that key was ever wired to, and its single caller passes nothing. The parameter is exercised only by `engine/infra/env.test.ts:45` — a dead parameter kept alive by its own test, which is precisely the shape that masks the `ts-prune`/`knip` gate. + +3. `conventions.patterns.taskId: 'T[0-9]{8}-[0-9]{6}'` and `findingPrefix: "F"` (`defaults.yaml:60-62`). `loader.ts:49` copies the block into `ConventionsConfig.patterns`, and no file reads it back — grep for `.patterns.` across `engine/`, `packages/`, `app/` returns only that one assignment; every other hit is a `.test.ts` fixture. The `taskId` value is additionally wrong: `KVBackedKindRegistry.generateId` (`packages/adapters/src/kind-registry/kv-backed-kind-registry.ts:89-101`) mints `{idPrefix}{date}-{8 uppercase hex}`, so real ids like `T20260705-2BABD6A7` can never match a 6-digit-sequence regex. That the fixtures disagree with each other on the value (`T[0-9]{8}-[0-9]{6}` in `loader.test.ts:64` vs `T{DATE}-{SEQ}` in nine other test files) is direct evidence nothing depends on its semantics. `findingPrefix: "F"` duplicates `kinds.yaml` `finding.idPrefix: F`, which is the key the code actually reads (`file-backed.ts:346`, `closed-pr-recovery.ts:89`). + +**Fix**: Delete `workspace:` from `defaults.yaml` and the `workspace` block from `engineDefaultsSchema`; drop the `defaultWorkspaceBaseDir` parameter from `loadEnv` and its now-meaningless test case, leaving `WORKSPACE_BASE_DIR` → `{operatorDir}/repos` as the documented and only resolution path. Delete `conventions.patterns` from `defaults.yaml`, `engineDefaultsSchema`, `buildDefaults`, `ConventionsConfig`, and `PatternConventions` in `packages/core/src/types/config.ts`, then drop `patterns:` from the ten test fixtures that carry it. Id format stays owned by the kind registry (`idPrefix` in `kinds.yaml`) — the single source of truth. Bump the seeded document in the same change; `engine-defaults` is seeded with `overwriteContentOnBoot: true` (`engine/storage/seed.ts:67`), so existing installs re-sync on next boot with no migration step. + +**Acceptance Criteria**: +- [ ] `grep -rn "baseDir\|findingPrefix\|patterns\.taskId" engine packages app --include=*.ts --include=*.yaml` returns no hits outside `kinds.yaml`'s `idPrefix` +- [ ] `engineDefaultsSchema` parses the shipped `defaults.yaml` with no unconsumed keys; a schema test asserts the parsed key set equals the set `buildDefaults` reads, pinning the structure so a future dead key trips the test +- [ ] `loadEnv` takes one argument; `engine/infra/env.test.ts` covers `WORKSPACE_BASE_DIR` set and unset, and the unset case asserts `{operatorDir}/repos` +- [ ] `docs/deployment.md:19` still matches the implemented default after the change +- [ ] `npm run typecheck && npm run lint && npm test` pass, with `ts-prune`/`knip` clean +