fix(verify): skipped results must not demote; add --no-demote raise-only mode#129
Merged
Conversation
CreateWorktree/RemoveWorktree shelled out to `git worktree` with the process
environment inherited verbatim. Git exports GIT_DIR, GIT_INDEX_FILE, and
GIT_WORK_TREE into hook and alias subprocesses, and when they leak into a
`git worktree add` child git resolves the new worktree's git dir/index against
the PARENT repository and fails ("fatal: .git/index: index file open failed:
Not a directory"). This makes the worktree helpers — and the repo's own
pre-commit hook, which runs `go test ./...` including the worktree tests —
fail whenever rtmx is invoked from a git-driven context on a git that exports
those variables.
Strip the per-invocation GIT_* variables before invoking git so it computes
the worktree's own paths from cmd.Dir. The existing worktree tests now pass
under a hook-like environment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A skipped test result carried no representation in the cross-language results schema: Result exposed only Passed bool, and the decoder folded status:"skip" (and any record supplying neither passed nor status) into Passed=false. The --results status engine then counted every non-passing record as a failure, so under require_all_pass (the default) a single skipped test DEMOTED a COMPLETE requirement to PARTIAL, and the shared count loop put the skip in TestsFailed — making `rtmx verify --results` exit non-zero even without --update. This was asymmetric with the native go-test path (determineNewStatus), which keeps current status on a skip. Sharded/matrix CI that skips a test in one leg therefore silently corrupted status on every run. Add a Skipped tri-state to Result and decode status:"skip"/"skipped", an explicit "skipped":true, and an absent-outcome record as skipped (non-evidence) rather than a silent failure — deliberately not a hard decode error, so one odd record cannot abort a whole best-effort results file. Both verify count loops now treat skip as non-evidence: it neither promotes nor demotes and is counted as TestsSkipped, not TestsFailed. A genuine failure still downgrades. Adds REQ-VERIFY-012 and the demotion-direction tests that were missing (COMPLETE + N combos + a skip stays COMPLETE; decoder skip/absent-outcome cases), which the prior suite never covered — it only tested forward promotion from MISSING, so the demotion regression shipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ult sets
Even with skipped results correctly ignored (REQ-VERIFY-012), a partial or
sharded result set legitimately carries fewer passing (scope, technique)
combinations for a requirement than its full validation does — because the
other combinations ran in a different CI leg or an environment filtered out of
this run. Under the combinations policy the engine correctly returns PARTIAL
for that partial view, so `--update` would demote a requirement that is
genuinely COMPLETE across all legs.
Add `--no-demote`: it computes the new status as usual, then keeps whichever of
{current, computed} is more complete (by Status.Weight), so a result set is
treated as additive evidence — it may raise a requirement's status but never
lower it. Without the flag, behavior is unchanged. Adds REQ-VERIFY-013 and
TestClampNoDemote.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
RTM ChangesHealth Status: [?] unknown RTM Database ComparisonStatistics
Added
Summary: IMPROVED
Generated by rtmx PR check |
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
rtmx verify --resultssilently corrupted requirement status on the cross-language path: a skipped test demoted a COMPLETE requirement to PARTIAL and made verify exit non-zero. This PR fixes that, adds a raise-only--no-demotemode for sharded/partial CI, and fixes a related git-worktree env-leak bug that made the repo's own pre-commit hook flaky.Found while wiring results-based promotion in a downstream consumer (the Phoenix radar RTM, which uses
completeness: {policy: combinations, min_combinations: 3}): requirements with three passing(scope,technique)combos were being demoted. rtmx's own dogfood RTM doesn't use thecombinationspolicy, so the demotion direction was never exercised.The bug (REQ-VERIFY-012)
The results
Resulttype had no representation for skip: the decoder foldedstatus:"skip"— and any record supplying neitherpassednorstatus— intoPassed=false. Then:determineStatusWithPolicycounted every non-passing record asfailed, so underrequire_all_pass(default) branch (A) demoted COMPLETE→PARTIAL on a single skip;TestsFailed, sortmx verify --resultsexited 1 even without--update.This is asymmetric with the native go-test path (
determineNewStatus), which keeps current status on a skip, and withfrom-pytest, which omits skips. Sharded/matrix CI that skips a test in one leg (e.g. a config variant) therefore corrupted status on every run.Fix: add a
Skippedtri-state toResult; decodestatus:"skip"/"skipped", explicit"skipped":true, and absent-outcome records as skipped (non-evidence) — not a hard decode error, so one odd record can't abort a best-effort file. Both verify count loops treat skip as non-evidence (TestsSkipped, neverTestsFailed). A genuine failure still downgrades.New:
--no-demoteraise-only mode (REQ-VERIFY-013)Even with skips ignored, a partial result set legitimately carries fewer combos for a requirement than its full validation (other combos ran in another leg).
--no-demotetreats a result set as additive evidence: it computes the new status, then keeps whichever of {current, computed} is more complete (Status.Weight) — raising freely, never lowering. Default behavior unchanged.Also: git-worktree env leak
internal/orchestration/{Create,Remove}Worktreeshelled out togit worktreewith the environment inherited verbatim. WhenGIT_DIR/GIT_INDEX_FILEare exported (as git does for hooks),git worktree addresolved the new worktree against the parent repo and failed (fatal: .git/index: index file open failed). This made the worktree tests — and thus the repo's ownpre-commithook (go test ./...) — fail when run from a git-driven context. Fixed by stripping the per-invocationGIT_*vars before invoking git.Tests
Adds the demotion-direction cases the suite never had (all prior combos cases used
current: MISSING):status:"skip"/"skipped"/"skipped":true/no-outcome → Skipped;status:"fail"unchanged.TestClampNoDemotefor the raise-only rule.Verified end-to-end against the real downstream data: the old binary demotes
REQ-VAL-015/REQ-SEC-002and exits 1; this branch leaves them COMPLETE and exits 0.Commits
fix(orchestration): don't leak GIT_* env into git worktree subcommandsfix(verify): treat skipped results as non-evidence on the --results path (REQ-VERIFY-012)feat(verify): add --no-demote raise-only mode (REQ-VERIFY-013)🤖 Generated with Claude Code