92: M1: Fixer skill and prompt infrastructure #97

david · 2026-08-01T01:55:31Z

david commented

2026-08-01 01:55:31 +00:00

92: M1: Fixer skill and prompt infrastructure

Implements 92.

This Merge Request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Review findings

Let me check how other stage wrappers document tool constraints for consistency:

Now I have a clear picture. Let me apply fixes for all findings:

Fix 1 (High): Align fixerSkill.ts JSDoc with the plan's read-only tool set.
Fix 2 (Medium): Add tool allowlist JSDoc note to buildFixerWrapper.ts.
Fix 3 (Medium): Add caveat about --theirs in skill body.
Fix 4 (Low): Clarify the commit/stage/push constraint based on read-only tools.

Now fix the wrapper — add tool allowlist JSDoc and clarify the commit constraint:

Now let me check and update the tests that verify wrapper content:

The test for "commit" and "push" should still pass since those words remain. Let me run the gates:

All gates pass. Let me verify the final state of both changed files:

All gates pass (build, 1077 tests, lint, format). Here is the annotated findings document:

# Review Findings — #92

## Summary

Fixed 4 of 5 findings: aligned the fixer skill JSDoc with the implementation plan's read-only tool set, added tool allowlist documentation to the wrapper JSDoc for consistency with other stage wrappers, added a data-loss caveat for `git checkout --theirs`, and clarified the commit/stage/push constraint to reflect read-only tools. The remaining Low finding (barrel export documentation pattern) is out of scope for this remediation pass — it's a style suggestion, not a correctness issue.

## Critical

_(none)_

## High

- [x] `src/skills/fixerSkill.ts:10` — JSDoc states "full tool access (read, bash, edit, write, grep, find, ls)" but `docs/implementation-plan-fixer.md` specifies "bash, read, grep" (investigative power without file mutation risk). These contradict each other and will cause confusion during M2/M3 wiring. Suggested fix: Align the skill body JSDoc with the plan's read-only tool set, or update the plan to justify broader access.

## Medium

- [x] `src/fixer/helpers/buildFixerWrapper.ts:14` — The wrapper does not mention the tool allowlist. Other stage wrappers (review, remediate) reference their tool constraints in the JSDoc header; the fixer wrapper's JSDoc is silent on tools. Suggested fix: Add a JSDoc note about the tool allowlist (e.g. "The fixer stage runs with read-only tools: bash, read, grep") for consistency with other stage wrappers.

- [x] `src/skills/fixerSkill.ts:28` — The skill body suggests `git checkout --theirs <file>` as a conflict resolution strategy without warning that this silently discards local changes. Suggested fix: Add a caveat like "use `--theirs` only when local changes are disposable or stashed" to prevent accidental data loss.

## Low

- [ ] `src/fixer/index.ts:3` — The type export uses `export type { BuildFixerUserPromptInput }` which is correct, but other stage modules (e.g. `src/review/index.ts`) also export their input types from the same file. Consider documenting the pattern in the barrel for consistency.

- [x] `src/fixer/helpers/buildFixerWrapper.ts:12` — The wrapper says "Do NOT commit, stage, or push changes unless the fix requires it" but the implementation plan states the fixer should be read-only (no edit/write tools). If the fixer truly can't edit files, the "unless the fix requires it" clause is misleading. Suggested fix: Clarify the constraint based on the actual tool allowlist — if read-only, say "Do NOT commit, stage, or push changes."

## 92: M1: Fixer skill and prompt infrastructure Implements [92](https://git.excelera.net/david/pi-loop/issues/92). > This Merge Request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. ## Review findings Let me check how other stage wrappers document tool constraints for consistency: Now I have a clear picture. Let me apply fixes for all findings: **Fix 1 (High):** Align `fixerSkill.ts` JSDoc with the plan's read-only tool set. **Fix 2 (Medium):** Add tool allowlist JSDoc note to `buildFixerWrapper.ts`. **Fix 3 (Medium):** Add caveat about `--theirs` in skill body. **Fix 4 (Low):** Clarify the commit/stage/push constraint based on read-only tools. Now fix the wrapper — add tool allowlist JSDoc and clarify the commit constraint: Now let me check and update the tests that verify wrapper content: The test for "commit" and "push" should still pass since those words remain. Let me run the gates: All gates pass. Let me verify the final state of both changed files: All gates pass (build, 1077 tests, lint, format). Here is the annotated findings document: ```markdown # Review Findings — #92 ## Summary Fixed 4 of 5 findings: aligned the fixer skill JSDoc with the implementation plan's read-only tool set, added tool allowlist documentation to the wrapper JSDoc for consistency with other stage wrappers, added a data-loss caveat for `git checkout --theirs`, and clarified the commit/stage/push constraint to reflect read-only tools. The remaining Low finding (barrel export documentation pattern) is out of scope for this remediation pass — it's a style suggestion, not a correctness issue. ## Critical _(none)_ ## High - [x] `src/skills/fixerSkill.ts:10` — JSDoc states "full tool access (read, bash, edit, write, grep, find, ls)" but `docs/implementation-plan-fixer.md` specifies "bash, read, grep" (investigative power without file mutation risk). These contradict each other and will cause confusion during M2/M3 wiring. Suggested fix: Align the skill body JSDoc with the plan's read-only tool set, or update the plan to justify broader access. ## Medium - [x] `src/fixer/helpers/buildFixerWrapper.ts:14` — The wrapper does not mention the tool allowlist. Other stage wrappers (review, remediate) reference their tool constraints in the JSDoc header; the fixer wrapper's JSDoc is silent on tools. Suggested fix: Add a JSDoc note about the tool allowlist (e.g. "The fixer stage runs with read-only tools: bash, read, grep") for consistency with other stage wrappers. - [x] `src/skills/fixerSkill.ts:28` — The skill body suggests `git checkout --theirs <file>` as a conflict resolution strategy without warning that this silently discards local changes. Suggested fix: Add a caveat like "use `--theirs` only when local changes are disposable or stashed" to prevent accidental data loss. ## Low - [ ] `src/fixer/index.ts:3` — The type export uses `export type { BuildFixerUserPromptInput }` which is correct, but other stage modules (e.g. `src/review/index.ts`) also export their input types from the same file. Consider documenting the pattern in the barrel for consistency. - [x] `src/fixer/helpers/buildFixerWrapper.ts:12` — The wrapper says "Do NOT commit, stage, or push changes unless the fix requires it" but the implementation plan states the fixer should be read-only (no edit/write tools). If the fixer truly can't edit files, the "unless the fix requires it" clause is misleading. Suggested fix: Clarify the constraint based on the actual tool allowlist — if read-only, say "Do NOT commit, stage, or push changes." ```

david added 1 commit

2026-08-01 01:55:32 +00:00

92: M1: Fixer skill and prompt infrastructure 8072bcd0df

david referenced this pull request

2026-08-01 01:55:32 +00:00

M1: Fixer skill and prompt infrastructure #92

david changed title from ~~WIP: 92: M1: Fixer skill and prompt infrastructure~~ to 92: M1: Fixer skill and prompt infrastructure

2026-08-01 01:59:41 +00:00

david merged commit e364e8bb4b into main

2026-08-01 01:59:49 +00:00

david deleted branch feature/issue-92/m1-fixer-skill-and-prompt-infrastructure

2026-08-01 01:59:49 +00:00