fix: the review findings on v0.11.1..HEAD (guard, review state, backlog, docs)

[sebastian-iancu]

Summary

Fixes what a whole-range review of v0.11.1..HEAD found:

• six review agents: code, error handling, test coverage, skills, cross-file consistency and docs;
• a second opinion on the fixes;
• two outside reviews.

The branch is stacked on #50 and targets main. Until #50 merges, the diff here also shows #50's commit; merging #50 first leaves only the two commits below.

Critical. sdd-pr guard built the target with os.path.join(tree, path). An absolute path, or a ../ one, therefore edited the user's checkout instead of the scratch worktree. The restore then failed and left the guard removed. Now:

• an absolute path under the root is made relative;
• any path leading out of the repository, or out through a symbolic link, is refused before anything runs.

guard.

• --expect must appear exactly once on the line, and that is checked before the unchanged run. An ambiguous --expect used to remove the wrong guard.
• The file is read and written as bytes, so encoding and CRLF survive.
• Files a run leaves behind are cleaned, so they can't fail the next run.
• A run past --timeout (600 s) is killed and counts as a failure.
• It refuses when a PYTHONPATH or NODE_PATH entry in the checkout would load the unchanged file.
• record and --cleanup wait on guard's lock, and --cleanup prunes a worktree whose folder was deleted by hand.

Review state.

• pull records a thread with no By: line as the maintainer's review, a maintainer line at HEAD. It also reads a trailing · outside, which the panel prompt asks for.
• status steers to --since-last only after the maintainer's review, and counts the pass budget from it. Before, the first triage could spend the one re-review before the maintainer had looked.
• A draft with nothing else open now points at /sdd-deliver --close-out instead of "mark the pull request ready", which skipped the close-out.

Scope.

• --since-last after a merge of the base that had conflicts uses the tree merge-tree still writes (exit 1). On git older than 2.38 it now says that the base's changes are in the range, instead of including them silently.
• scope --start refuses with pass: running (exit 2), and its note is JSON, so an agent name containing a space no longer breaks it.

Backlog.

• harvest runs with forge: none and only names the lines to re-check. Before, nothing deleted the lines a branch settled there, since the close-out no longer deletes them.
• The re-check reads merge commits (--first-parent -m) and renames (--name-status).
• carry re-sorts the whole Unsorted section, so a defect carried later still leads.

Findings file.

• add folds one defect graded twice into one line: the higher severity, and outside only when both reports say so.
• - [ ] suggestion reads as a suggestion. Before, it was posted as an inline thread, a regression from v0.11.1.
• flip --suggestions --deferred takes the outside lines too.
• record keeps its line when the cleanup fails.

Instructions.

• /sdd-review --panel runs step 2, which decides the lane it reads.
• The close-out runs "when nothing blocks, a draft aside". The old "nothing blocking but a draft" read as "requires a draft".
• The conformance reviewer grades a MUST with no test as untested (critical), and falls back to suggestion when unsure only about what isn't code behaviour.
• The panel prompt asks a reviewer working on the pull request to put an outside defect in the review body; GitHub can't anchor an inline comment outside the diff. ai-workflow.md § Review states code first.
• Methodology §9 is now the one home for the close-out after merge, and its rules table names outside.
• The session-start hook creates .sdd/ for the skills' output redirects (git ignores it while empty) and names the plugin root, which the new sdd-pr permission rule in install.md needs.

Declined:

• The reviewers' ten-suggestion cap. It is per reviewer and still documented; 0.11.0 removed only the per-pass cap.
• Build errors reported as pinned. A language-agnostic tool can't tell a compile error from a failing test; the output says so, and the reviewer is told to use --replace so the code still builds.
• The budget counting passes on commits a rebase dropped. A rebase doesn't change what was reviewed.
• A fix to documents alone not steering Next. The tool can't tell it from the close-out's own commit.
• The comment header counting a resolved outside line. It counts lines; the summary counts the open ones.

Docs

• References: review.md (§ The findings file, § The forge mirror, § The backlog, § Passes, § The tool) and sdd-methodology.md (§9, the rules table).
• Skills:
  • sdd-review: steps 5 and 6;
  • sdd-deliver: step 10;
  • sdd-triage: step 6 and --backlog.
• Agents: sdd-spec-conformance-reviewer.
• Templates: ai-workflow.md.
• Hooks: session-start.sh.
• Docs: install.md, examples.md (the status sample now matches the tool's real output), quick-start.md, testing.md, README, AGENTS.md and CHANGELOG.

Word budgets grow:

File	Words added
sdd-review	+6
review.md	+121
conformance reviewer	+15
ai-workflow.md	+48

Verification

• ./scripts/validate.sh: OK (word budgets, forge-neutral text, versions); hooks-test OK, including two new checks (the plugin root, .sdd/).
• python3 -m unittest discover -s tools/tests: 540 tests OK (was 512). python3 tools/sdd-check.py selftest: OK, 26 cases.
• claude plugin validate .: passed.
• Can-fail proof: I broke each fix in turn in a scratch copy of the tool (19 versions), and each one failed at least one test.
• Real git, in a throwaway repository:
  • an absolute path mutates the scratch copy and leaves the checkout clean;
  • a CRLF file keeps its line endings;
  • ../../etc/hosts is refused (exit 2);
  • after a merge of the base with a conflict, --since-last lists the branch's fix and the conflict resolution and leaves out the base's new file.

Notes for review

Look hardest at:

• the new pass-budget signal in cmd_pull and cmd_status;
• diff_start's handling of exit 1.

A decision for you. Methodology §13's merge gate asks for a guard-removal test on every binding sentence the change touches. The conformance reviewer proves only MUST and MUST NOT. Either extend guard to SHOULD and SHOULD NOT (more test runs per pass), or narrow §13's wording. Not changed here.

Known gap: the remove_scratch lock is not pinned by a test.

Checklist

• Scoped to one logical change (the review's findings)
• The gates above pass locally and their output was read
• CHANGELOG entry under Unreleased
• AGENTS.md, README.md and hooks/session-start.sh synced; no component added or renamed
• No internal or private repository names, absolute paths or organisation-private details

🤖 Generated with Claude Code