Phase 21: polish write-mode smoke docs + guardrail UX#20
Conversation
Document xbmc/xbmc write-mode smoke steps and evidence grepping, and capture Phase 21 plan artifacts/state.
Enrich write-policy errors with rule/path/detector context, include that detail in refusal replies, make rereview commands request the UI team without posting comments, and raise the default execution timeout for large repos.
![kodiai[bot]](https://avatars.githubusercontent.com/in/2832814?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Phase 21 polish rollup improving write-mode UX/guardrails, adding ops docs for xbmc/xbmc smoke testing + log grepping, introducing a non-chatty rereview trigger, and increasing default execution timeouts to reduce large-repo failures.
Changes:
- Enrich write-policy refusals with rule/path/pattern/detector context and ensure same-repo PR-branch update path surfaces refusals cleanly.
- Add minimal
@kodiai review/@kodiai recheckrereview trigger via UI reviewer request (no PR-thread comments). - Increase default execution timeout to 600s and update timeout guidance to reference
timeoutSeconds; make CI typecheck blocking; add smoke/runbook docs.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/sanitizer.test.ts | Adjusts expectations to satisfy stricter TS indexing checks. |
| src/lib/errors.ts | Updates timeout suggestion text to reference timeoutSeconds. |
| src/jobs/workspace.ts | Adds structured metadata to WritePolicyError and improves guardrail error context (pattern/detector/path). |
| src/handlers/mention.ts | Implements rereview command via reviewer request; improves write-policy refusal replies (including PR-head update path). |
| src/handlers/mention.test.ts | Adds/refines tests for refusal detail fields and rereview behavior. |
| src/execution/executor.ts | Raises runtime default timeout to 600s (used if config load fails). |
| src/execution/config.ts | Raises config default timeoutSeconds to 600s. |
| docs/smoke/xbmc-xbmc-write-flow.md | Adds end-to-end write-mode smoke procedure for xbmc/xbmc. |
| docs/runbooks/mentions.md | Adds runbook snippet for grepping evidence bundle logs by deliveryId. |
| .planning/phases/21-polish/21-01-PLAN.md | Adds plan doc for Phase 21 plan 01. |
| .planning/phases/21-polish/21-01-SUMMARY.md | Adds summary for Phase 21 plan 01. |
| .planning/phases/21-polish/21-02-PLAN.md | Adds plan doc for Phase 21 plan 02. |
| .planning/phases/21-polish/21-02-SUMMARY.md | Adds summary for Phase 21 plan 02. |
| .planning/phases/21-polish/21-03-PLAN.md | Adds plan doc for Phase 21 plan 03. |
| .planning/phases/21-polish/21-03-SUMMARY.md | Adds summary for Phase 21 plan 03. |
| .planning/phases/21-polish/21-04-PLAN.md | Adds plan doc for Phase 21 plan 04. |
| .planning/phases/21-polish/21-04-SUMMARY.md | Adds summary for Phase 21 plan 04. |
| .planning/STATE.md | Updates planning state to Phase 21 progress. |
| .github/workflows/ci.yml | Makes typecheck step blocking by removing continue-on-error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| denyMatchers = compileGlobMatchers(denyPaths); | ||
| allowMatchers = compileGlobMatchers(allowPaths); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| throw new WritePolicyError( | ||
| "write-policy-not-allowed", | ||
| `Write blocked: invalid path policy pattern: ${message}`, | ||
| { rule: "allowPaths" }, | ||
| ); |
There was a problem hiding this comment.
The invalid-glob catch block always sets meta.rule to "allowPaths", but the failure could be caused by a denyPaths pattern as well. This can mislead users when troubleshooting (the message will point at the wrong rule family). Consider compiling/validating denyPaths and allowPaths separately so you can attribute the error to the correct rule (or include both).
| lines.push(` - '${err.path}'`); | ||
| lines.push("```"); | ||
|
|
||
| if (allowPaths.length > 0) { | ||
| lines.push(""); | ||
| lines.push(`Current allowPaths: ${allowPaths.map((p) => `'${p}'`).join(", ")}`); |
There was a problem hiding this comment.
The refusal message prints a YAML snippet using single quotes around err.path and allowPaths entries. Git paths can legally contain a single quote, which would make the suggested YAML invalid. Consider escaping single quotes for YAML single-quoted scalars (e.g., replace ' with '') or using a safer quoting strategy when interpolating paths/patterns into the snippet.
| lines.push(` - '${err.path}'`); | |
| lines.push("```"); | |
| if (allowPaths.length > 0) { | |
| lines.push(""); | |
| lines.push(`Current allowPaths: ${allowPaths.map((p) => `'${p}'`).join(", ")}`); | |
| const escapedPath = err.path.replace(/'/g, "''"); | |
| lines.push(` - '${escapedPath}'`); | |
| lines.push("```"); | |
| if (allowPaths.length > 0) { | |
| lines.push(""); | |
| lines.push( | |
| `Current allowPaths: ${allowPaths | |
| .map((p) => `'${p.replace(/'/g, "''")}'`) | |
| .join(", ")}` | |
| ); |
| }); | ||
|
|
||
| describe("createMentionHandler rereview command", () => { | ||
| test("@kodiai review posts minimal reply and requests aireview team", async () => { |
There was a problem hiding this comment.
This test name says it "posts minimal reply", but the test setup explicitly throws if createReplyForReviewComment is called (i.e., it expects no PR-thread comment). Rename the test to reflect the actual behavior (requesting reviewers without posting a comment) to avoid confusion.
| test("@kodiai review posts minimal reply and requests aireview team", async () => { | |
| test("@kodiai review requests aireview team without posting a reply comment", async () => { |
| - `evidenceType=write-mode` | ||
| - `outcome=updated-pr-branch|created-pr|reused-pr|skipped-idempotent` | ||
| - `deliveryId=<X-GitHub-Delivery>` |
There was a problem hiding this comment.
The evidence-bundle log field is emitted as a structured JSON key deliveryId (see logger.info calls in handlers), not as a deliveryId=<...> string. Using deliveryId=<X-GitHub-Delivery> here may mislead grep/query guidance; consider aligning with the actual field/key format used in logs (e.g., deliveryId: <...> / deliveryId key).
| - `evidenceType=write-mode` | |
| - `outcome=updated-pr-branch|created-pr|reused-pr|skipped-idempotent` | |
| - `deliveryId=<X-GitHub-Delivery>` | |
| - `"evidenceType": "write-mode"` | |
| - `"outcome": "updated-pr-branch|created-pr|reused-pr|skipped-idempotent"` | |
| - `"deliveryId": "<X-GitHub-Delivery>"` |
Split path-policy glob validation by rule family, escape YAML suggestions safely, align smoke docs with structured log fields, and clarify rereview test name.
|
@kodiai recheck |
kodiai responseDecision: NOT APPROVED Issues:
|
|
@kodiai recheck |
kodiai responseDecision: NOT APPROVED Issues:
|
Reuse the existing installation Octokit for replies/errors/reactions to avoid redundant client creation. Ensure rereview commands always attempt the default aireview team when uiRereviewTeam is unset/blank.
|
@kodiai recheck |
kodiai responseDecision: APPROVE Issues: none |
This rolls up Phase 21 polish:
@kodiai review/@kodiai recheckretrigger review via UI reviewer request without adding PR-thread comments.Verification: