Fix the three extraction regressions: false-green whole-repo check, review workspace root, displayed defaults - #6
Closed
schickling-assistant wants to merge 3 commits into
Conversation
schickling
marked this pull request as ready for review
August 9, 2026 08:28
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ee2cc2afe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
schickling-assistant
marked this pull request as draft
August 9, 2026 09:49
…oots `meta_vrs_decision_dir` resolved only `<root>/.decisions`, so `check <repo-root>` found no decision directory, skipped the shape pass and reported a clean tree — a malformed decision under `context/vrs/.decisions/` passed. A false green is the worst failure mode an enforcement tool has: it is indistinguishable from a run that found nothing to report. The fallback is strictly additive. It is consulted only when `<root>/.decisions` is absent, and it names a path a differently laid out repository does not have, so it can only add enforcement, never remove or redirect it. No test could have caught this: every helper on both sides passes `context/vrs` directly, which resolves the decision directory however the tool locates it. The new test aims `check` at the repository root instead, and fails on unfixed code with an empty diagnostics array. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
…kspace `review_workspace` probed only for `.git`, so a tree without one resolved to the corpus root. That root becomes the review backend's `--cwd`, so the reviewer was handed the corpus alone and lost the repository the corpus describes. Ordinary worktrees have `.git` and mask this entirely, which is why no interactive run reaches it — only a vendored or exported tree, or a build sandbox, does. Neither probe is a guess that can mislead: both name a directory that is present or is not, and a repository laid out differently falls through to the corpus as before. That last-resort branch keeps its test; only its stale comment moved. The reported escape-boundary consequence does NOT reproduce, and the report is corrected here rather than repeated: target artifacts are collected from the corpus root, and the workspace is always that root or an ancestor of it, so the boundary check cannot reject them under either resolution. The corpus position is now one named constant shared with the decision-directory lookup, so the two cannot drift apart. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
The corpus defaults moved out of the command tree so the standalone tool and its embedding host could carry different ones, and were applied after parsing instead. Bare invocation still resolved correctly, but `check|graph|review|review-fixtures --help` stopped advertising them and generated completion metadata lost them — so the documented default and the displayed one could disagree with nothing failing. Caller-supplied defaults do not actually require giving up the displayed metadata: `Defaults::command` injects them onto the arguments themselves, and `Defaults::parse` parses through that same tree. The binary now parses that way, so what `--help` advertises is what the run will use. `run_with` still resolves an absent argument, so callers that build the types directly are unaffected. This needs clap's `string` feature: the defaults are runtime values, and without it `default_value` takes only `&'static` ones — which is the constraint that pushed them out of the command tree to begin with. Both tests fail without the change. The integration test drives the SHIPPED binary, because a test that built its own command tree would reproduce the blind spot rather than catch it; the unit test covers an embedding caller's own default. agent-tool: Claude Code agent-tool-version: 2.1.220 agent-runtime: Claude Code 2.1.220 agent-session-lookup: unavailable tooling-profile: dotfiles@unknown-dirty
schickling-assistant
force-pushed
the
schickling-assistant/2026-08-08-fix-extraction-regressions
branch
from
August 9, 2026 09:54
3ee2cc2 to
18299fe
Compare
schickling-assistant
marked this pull request as ready for review
August 9, 2026 10:17
schickling-assistant
marked this pull request as draft
August 9, 2026 14:49
schickling
marked this pull request as ready for review
August 9, 2026 15:31
Contributor
Author
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
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.
Fixes the three behaviours reported in #4. One of them was a false green; the other two
are reported here with what actually reproduced and what did not.
1. Whole-repository
checkreturned a false green (high)meta_vrs_decision_dirresolved only<root>/.decisions, socheck <repo-root>found nodecision directory, skipped the shape pass entirely and reported a clean tree. Restored the
fallback to the conventional corpus position.
The fallback is strictly additive: it is consulted only when
<root>/.decisionsisabsent, and it names a path a differently laid out repository does not have. It can only
ever add enforcement, never remove or redirect it — so the "silently wrong for another
layout" objection in the removed comment does not hold: such a repository has no
context/vrs/.decisions, and nothing changes for it.Scope question, flagged rather than decided. #4 frames this as the contract documenting
whole-repository scope while the code implements corpus scope. I could not find that text.
Searching this repository for whole-repository/repo-wide/repository-root phrasings turns up
nothing that says
checkat a repository root enforces decision shape, and the surroundingmaterial leans corpus-scoped:
context/cli/spec.md:63documents the defaultpathascontext/vrs, and the argument's own help has always read "VRS root to check". The onerepository-relative statement is
context/cli/spec.md:174, which names the rule's subject ascontext/vrs/.decisions/.So the contract is not in conflict here, and this is a documentation clarification rather than a
choice: the restored fallback consults both paths, so the whole-repository reading and the
narrower corpus-scoped line in
context/cli/spec.mdare each satisfied. Nothing needs to benarrowed. I did not touch
context/cli/*.md.Correction — "removes a false green either way" is too strong, and independent verification
falsified it. It restores the missing enforcement for the
context/vrslayout — genuinely a cleanpass on a bad tree in the harness above, though on a real populated tree the run fails on other
rules and the loss shows up as one silently absent rule rather than a green. It does not restore
it for this repository, because
CORPUS_SENTINELis hardcoded tocontext/vrswhile this repo's corpus isintent/. Aimed atthis repo's root, the decision-shape pass is still silently absent. That residual gap predates
this PR (the hardcode is already at
da5ac9f1b), is currently latent — all 36 records scan cleanand CI aims at the corpus — and is not a clean-bill-of-health green here, since the repo-root
run exits 1 on 8 other diagnostics. It is tracked separately as #8 and is deliberately out of
scope for this change.
The test is the point
decision_shape_is_enforced_when_check_is_aimed_at_the_repository_rootinvokescheckat therepository root with a malformed decision at
context/vrs/.decisions/. On unfixed code itfails with an empty diagnostics array — a clean pass on a tree containing a malformed
decision, which is exactly the reported failure.
Every pre-existing helper hands
checkthecontext/vrspath itself, which resolves thedecision directory however the tool locates it. That is why the regression was invisible, and
why a test on the corpus path would reproduce the blind spot instead of catching it. The new
check_athelper exists so the aimed-at path is a parameter rather than baked in.2.
review_workspacelost itscontext/vrssentinel (medium) — reproduced, partlyThe mechanism reproduces. With only the
.gitprobe, a corpus in a tree without.gitresolves to the corpus root rather than the repository root
(
review_workspace_finds_the_repository_by_corpus_layout_when_there_is_no_gitfails onunfixed code, returning the corpus). That root becomes the review backend's
--cwd, so thereviewer is handed the corpus alone and loses the repository the corpus describes. Restored
the sentinel as a second probe.
The escape-boundary consequence does not reproduce, and I would rather correct that here
than repeat it. #4 says target artifacts elsewhere in the repository would be rejected. They
would not: target files are collected by
markdown_files(invocation.root)(
crates/intent/src/lib.rs:432), which descends from the corpus root only, and the workspaceis always that same root or an ancestor of it. Every target file therefore starts with the
workspace under either resolution, so the check at
crates/intent/src/lib.rs:449cannot firefor
review. The reported harm is real; that particular mechanism for it is not.Neither probe is a guess that can mislead — both name a directory that is present or is not.
The last-resort corpus fallback keeps its existing test, unmodified; only its now-stale
comment changed.
3. Displayed defaults disappeared from
--helpand completions (medium)The defaults moved out of the command tree so the standalone tool and its embedding host could
carry different ones. That constraint is real, but it does not require giving up the displayed
metadata:
Defaults::commandinjects the caller's defaults onto the arguments themselves, andDefaults::parseparses through that same tree.check --helpnow reads[ROOT] VRS root to check [default: .]again, and completions carry it.run_withstill resolves an absent argument, so callers constructing the types directly areunaffected. Setting
default_valuemeansargs.rootnow arrives asSomeon the binarypath rather than
None, which changes nothing observable: it is read only throughroot_or_default/fixtures_or_default, and both resolve it to the same value either way.Half of this lands here, and the other half cannot.
--helpis rendered from whichevercommand tree the caller builds. The standalone binary now builds its through
Defaults::command, so its help is fixed and tested. An embedding host parsesVrsClithrough its own command tree and then calls
run_with, so its help gains the defaultsonly once it adopts
Defaults::command/Defaults::parsetoo — a one-line change, but onethat belongs in the host's repository, not this one. #4 reports the symptom across
check|graph|review|review-fixtures --help; this PR supplies the mechanism and fixes thestandalone surface, and the host side is a separate follow-up rather than something silently
covered here.
This needs clap's
stringfeature: the defaults are runtime values, and without itdefault_valueaccepts only&'staticones — which is the constraint that pushed them out ofthe command tree originally. No new dependency, and
Cargo.lockis unchanged.Two tests, both failing without the change:
help_advertises_the_default_root_the_run_will_usedrives the shipped binary. Thismatters: the defect is a default that exists after parsing but never reaches the command
tree
--helprenders, so a test building its ownCommandwould pass while the real--helpstayed empty. Verified by revertingmain.rsalone — the test fails.a_callers_default_reaches_the_help_it_renderscovers an embedding caller's owncontext/vrsdefault, which is the case the constraint was about.Noted, not fixed
crates/intent/src/lib.rsemitsschema_version: "axe.vrs.check.v1", butcontext/cli/spec.mddoes not mentionaxe.vrs.check.v1anywhere — its envelopeenumeration is incomplete. Left alone deliberately: a rename is in flight across those
strings and would conflict.
check_rootskips the decision-shape pass silently whenever no decisiondirectory is found (
crates/intent/src/lib.rs:1085). Restoring the fallback fixes thereported case, but "found no decisions anywhere" still exits 0 and is indistinguishable from
a clean tree. Making that observable needs a new diagnostic class, which is more than this
change should carry.
is not a contained change and is not attempted here.
context/vrsat two sites — decision-dir skip and repo-root discovery #8 — this repository's own layout is still not covered, and this PR does not cover it.CORPUS_SENTINELis hardcoded tocontext/vrs; this repo's corpus isintent/. Aimed at therepository root, decision-shape enforcement is silently absent — proven by execution at this head
with a planted malformed decision:
intent check .reports 0VRS.ENF.meta-decision-shapediagnostics where
intent check intentreports 4, same binary, same file. It is not awhole-run false green here: the repo-root run exits 1 on 8 unrelated diagnostics. The hardcode
predates this PR, it is latent today (all 36 records scan clean and CI aims at the corpus), and
the fix belongs with a fixture on this repo's shape rather than a generic corpus probe — decision
0022deliberately scopes strict enforcement to one designated corpus, and a "find whatever holdsVRS artifacts" probe is the broadening it rejected. Tracked in CORPUS_SENTINEL hardcodes
context/vrsat two sites — decision-dir skip and repo-root discovery #8.Verification
nix flake checkpasses locally —fmt,clippy,intent,help, andreads-the-corpus.22 tests green (10 unit, 12 integration).
Correction — "each new test was confirmed to fail on unfixed code" is not accurate for all of
them, and independent verification established which. Three genuinely fail against
da5ac9f1b:decision_shape_is_enforced_when_check_is_aimed_at_the_repository_root(empty diagnostics array),help_advertises_the_default_root_the_run_will_use, andreview_workspace_finds_the_repository_by_corpus_layout_when_there_is_no_git(returns the corpusrather than the repository root). The fourth,
a_callers_default_reaches_the_help_it_renders, does not compile against unfixed code — thereis no
commandmethod to call — so it cannot fail as a test. It is a real regression test goingforward; it is not evidence that the behaviour was broken before, and the original wording claimed
it was.
Posted on behalf of @schickling
agent_nameagent_toolagent_tool_versionagent_runtimeagent_session_lookuptooling_profile