Accepted-risks archive, project-instructions, and reliable Step 2#18
Merged
Conversation
- Add Config.AllowDangerousPermissions (default true) and gate --dangerously-skip-permissions on it in the opencode runner - Extract runExec helper to share subprocess wiring, timeout and log breadcrumbs between Claude and opencode runners - Add buildArgs() methods on each runner with unit tests - Pass ctx into ExecClaudeRunner.saveOutput for log consistency - Fix --author to read CI_COMMIT_AUTHOR (the actual change author, stable across retries) instead of GITLAB_USER_LOGIN, which only reflects whoever triggered the pipeline - Add authorName helper to strip email from "Name <email>" so it does not leak into Slack notifications and the public API - Add envBool helper to align --verbose, --debug-upload and the new --allow-dangerous-permissions behind one explicit-default pattern - Document --allow-dangerous-permissions in README key flags
- Reframe Step 2 as extraction from MD: model copies issues from ### LocalID. headings into review.json instead of treating it as a fresh task that often gets skipped after lengthy MD output - Add ✓ Шаг 1 / ✓ Шаг 2 echo checkpoints for diagnostic visibility in opencode-output.jsonl when a step stalls mid-run - Allow files[] to drop reviewTypes with no MD file in Step 1, so projects with fewer configured prompts produce a consistent files[]–MD pairing instead of fabricated summaries - Fall back to %TITLE% / %COMMIT_HASH% / %SOURCE_BRANCH% etc. placeholders in WriteReviewSkeleton when CI env vars are unset; prompt now spells out git commands the model uses to resolve them - Skip empty values in SubstituteVariables so unresolved placeholders in the prompt body survive for the model to fill from git context - Add warnIfReviewJSONUnfilled to surface the silent "model skipped Step 2" failure (uploaded skeleton with empty summaries)
- Forbid `"` and `\` in title/description/summary; instruct model to rephrase code-shaped predicates as natural language so a literal cr.SessionID == "" doesn't get over-escaped into \\\" and break JSON parsing - Add Read-tool self-validation step after each review.json save, with hint that broken parses usually trace to over-escaped quotes in description/title - Restructure CI-metadata placeholder list as bullets and clarify %EXTERNAL_ID% must always become "" in non-CI runs (model was leaving the literal placeholder in DB) - Restate isAccepted as a critical/high gate: medium/low issues no longer push isAccepted to false; observed reviews with no blocking severity still came back fully unaccepted - Add cross-check items 7-9 to the self-check list (JSON validity, externalId placeholder, isAccepted vs issues[] consistency)
- Add `archivedAt` column on issues with btree index via PGD/SQL regen + migration; mfd-generated ArchivedAtIsNull search powers the new IssueSearch.ExcludeArchived filter end-to-end (rpc → domain → db) so the Accepted Risks tab hides archived items - Add review.ArchiveAcceptedRisks RPC and ArchiveProjectAcceptedRisks manager method that bulk-marks all FP/Ignored issues of a project as archived; original statusId is preserved so unarchive is a single-column reset - Add Get Project Instructions: new REST endpoint /v1/rpc/project-instructions-:id renders a project's ignored issues into markdown grouped by reviewType (capped at 500 with truncation notice); frontend buildProjectInstructionsPrompt builds the LLM prompt that fetches and synthesizes project rules - Extract shared markdownTemplateFuncs so the safeBody helper that neutralises untrusted-data closing tags has a single definition used by fixmd and instructions_md templates - Add two buttons to the Accepted Risks tab header (copy LLM prompt, archive shown) and unit/integration tests covering grouping, injection neutralisation, truncation notice, and archive idempotency
- Detect runner skipping Step 2 (skeleton uploaded, no issues, all summaries blank) and auto-retry with --resume/-s on the previous sessionId so the original 16K prompt stays cached and only the ~3K Step 2 body re-bills as input - Extract promptStep2Body as the single source of truth for Step 2 rules (schema, severity, isAccepted, JSON safety, self-check); promptReviewJSON and PromptStep2Retry both include it so the two paths can never drift - Auto-upload debug bundle when skip detected even on otherwise- successful runs so the opencode-output.jsonl + MD files are kept for post-mortem instead of being overwritten by the next run - Add prompt rule that all R*.md files must land in cwd (not in docs/reviews/ or other subdirs); FindMDFiles is non-recursive and silently dropped MDs the model placed elsewhere - Add prompt rule scoping the runner to R*.md and review.json only; observed runs created stray helper test files because the prompt didn't explicitly forbid editing the codebase
- Add ReviewModelInfo.Add summing all numeric fields and merging the Models map; runStep2Recovery uses it to fold retry-pass cost and tokens into the primary record. Fixes silent drop of WebSearchRequests, WebFetchRequests, CacheCreate1h/5mInputTokens that the previous manual sum missed - Add SetSession to ReviewRunner so the retry path resumes session via interface method instead of type-switch — third runners get a compile error rather than silent no-op - Extract isReviewJSONUnfilled, runStep2Recovery, retryStep2 into recovery.go so ctl.go stays focused on Review orchestration - Move EnvBool, EnvDefault, AuthorName from main.go to pkg/reviewer/ctl/env.go for testability next to Config; envBool now uses strconv.ParseBool, accepting 1/0/t/f/true/false - Replace orPlaceholder with stdlib cmp.Or; expose CI placeholders as named constants shared between prompt.go and skeleton.go; mark legacy %MR_TITLE% as Deprecated - Add retried flag to "review completed" log so the retry path is observable in production logs - Surface ListIgnoredIssuesByProject truncation as a slog warning so projects exceeding the 500-issue cap don't decay silently - Add unit tests: ReviewModelInfo.Add (6 cases), isReviewJSONUnfilled + retryStep2 (10 cases), env helpers (22 cases), ProjectInstructionsMarkdown handler (8 cases) including projectKey-leak guard and end-to-end render of seeded ignored issues
CI runs `go test -test.run="Test[^D][^B]" ./...` to skip tests that need a live PostgreSQL connection. The new rest_test.go cases were named TestProjectInstructionsMarkdown_* and slipped through the filter, hitting "dial tcp [::1]:5432: connect: connection refused" on the runner. Rename to TestDBProjectInstructionsMarkdown_* per project convention (cf. TestDBReviewManager_*, TestDBProjectManager_*).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
archivedAtcolumn (PGD/SQL/migration + mfdArchivedAtIsNullfilter),ArchiveAcceptedRisksRPC, and a header button on the project's Accepted Risks tab. Original statusId is preserved so unarchive is a single-column reset./v1/rpc/project-instructions-:idrenders ignored issues into markdown grouped by reviewType (capped at 500 with truncation hint); frontend builds an LLM prompt that fetches and synthesizes project review rules.isReviewJSONUnfilledheuristic), calls the runner again with--resume/-sand a focused retry prompt. Original 16K prompt stays cached; only the ~3K Step 2 body re-bills. Retry metrics are folded into the primary record viaReviewModelInfo.Addso dashboards reflect total billable spend.SetSessioninterface method (no more type-switch),cmp.Orfor placeholders, env helpers extracted topkg/reviewer/ctl/env.go, debug-bundle auto-uploaded on Step 2 skip, "review completed" log gainsretriedflag, truncation surfaces as slog warning.Test plan
make fmt lint test— green (currently 0 lint issues)pgmigrator run—2026-05-08-archive-accepted-risks.sqlapplies to dev DB without errormake mfd-model— regenerates without spurious diffreviewctl review(Claude opus) — one-pass success, all metadata resolved from gitreviewctl review(opencode/qwen) — first pass skips Step 2 →review.json appears unfilledwarn → retry triggered →Step 2 retry filled review.jsoninfo → review uploaded with issues/reviews/project/:id/Accepted Risks tab — bothGet Project InstructionsandArchive shownbuttons work; archive empties tab; instructions prompt fetches markdown without leaking projectKeyGET /v1/rpc/project-instructions-:id.md— 200 with markdown, 400 on bad id, 404 on missing projectexcludeArchived: truefilter hides archived issues from the tab