Skip to content

feat(cli): name preflight refusal stages and selected profiles - #407

Merged
barbatdev merged 1 commit into
mainfrom
fix/qa-refusal-profile
Oct 30, 2026
Merged

barbatdev merged 1 commit into
mainfrom
fix/qa-refusal-profile

Conversation

@barbatdev

Copy link
Copy Markdown
Contributor

Linked Issue

Refs #374

Umbrella: #401

PR Type

  • type:feature Feature
  • type:bug Bug fix
  • type:docs Documentation
  • type:refactor Refactoring
  • type:chore Maintenance
  • type:breaking-change Breaking change

Summary

Name route resolution, actor admission, model probe and evidence admission refusals. Report the selected/default/valid profile ids and remove a misleading default-provider claim from the prerequisite note.

Changes

This is slice 6/8, 310 authored changed lines. Code, focused tests and applicable operating documentation stay together. Exact reviewed source commit: b337e8b6cb94c0d66daf99cafdd672d834d8de1c. Delivery head 569ea98a97fb90cb3eed86afdb917089c07fe3d8 has the identical complete source tree.

Focused Tests

Each cumulative prepared slice passed go test ./internal/cli ./internal/qapi ./internal/qaroute ./internal/installobserve -count=1, go vet ./... and whitespace checks. Final candidate additionally passed go test ./... -count=1, go vet ./..., go test -race ./... and all 15 repository Python policy tests on a byte-verified canonical clean snapshot. Independent source-agreement, typed-admission and final security verification passed with no severe blockers; final verification observed 19 named tests plus four package suites/vet.

Native assessment of this exact slice: medium native tier, reviewDue=false (under_budget); no new native review started. Security-affecting slices retained explicit high-risk escalation and independent verification rather than treating assessment as proof. Parser's separately requested native review completed approved and acknowledged.

Runtime Harness

Disposable public CLI fixtures, no real provider calls. Rejections pin no launch/write effects, route identity and safe profile tokens. macOS /tmp is a symlink and intentionally refused: canonical snapshot cwd passed unchanged guards. No credentials changed.

Rollback

Revert this slice. Revert any dependent later slice first; no stored-data migration.

Chain Dependencies / Out of Scope

Sequential verified-main strategy: merged #406 -> [current slice 6/8] -> remaining slices. This PR targets main only after its predecessor lands, so its diff contains only this work unit. Do not close the umbrella from this intermediate slice.

Known acceptance limit: #329's literal transient/persistent causal distinction is NOT established by stage reporting and remains open. Defensive untyped branches, a currently unreachable non_nan_route mapping and a brittle wording assertion were disclosed as nonblocking verifier advisories, not silently fixed. Provider transport #345 and lifecycle #373 are out of scope.

Checklist

  • Linked issue is open and status:approved.
  • Exactly one matching type:* label is applied.
  • Focused tests and runtime harness evidence are recorded.
  • Rollback and out-of-scope boundaries are documented.

@barbatdev barbatdev added the type:bug Bug fix label Oct 30, 2026
@barbatdev
barbatdev merged commit f06fc84 into main Oct 30, 2026
3 checks passed
@barbatdev barbatdev mentioned this pull request Oct 30, 2026
5 of 10 tasks
@barbatdev

Copy link
Copy Markdown
Contributor Author

Fleet review (deepseek)

Lens: evidence + test design. The slice does what its title says for the main refusal path: qa run now prints a fixed stage= line for route resolution, actor admission, model probe and evidence admission, a profile_invalid refusal enumerates profile_selected/profile_default/profile_valid_ids through the same safe snapshot the run uses (qapi.ProfileSelection -> qaroute.ProfileIDs/DefaultProfileID, ids validated by ^[a-z][a-z0-9-]{0,63}$, so no injection through those fields), and the misleading placeholder-route sentence is gone from note=prerequisites. Two blockers stop it: the new docs/operating.md section promises a stage line for pre-launch refusals that still print none (evidence sidecar read failure, install-root failure), and it promises route_provider/route_model are "escaped for the terminal" while doctor.go still writes the profile-supplied pair raw. The tests are meaningful where they exist (exact stderr, no-launch artifacts, fixture fingerprint) but the new documented fields profile_source=unusable, profile_selected and profile_default have no assertion, the escaping claim has none, and one legacy assertion is now vacuous. Reviewer ran no code from the PR; surrounding sources were read read-only at the reviewed head.

Blocking (2)

  • Pre-launch refusals that return before the stage block still print no stage (internal/cli/doctor.go:387): qaRefusalStage is only consulted in the block that runs after qapi.RunLocalReportOn returns (doctor.go:386-389). Every earlier pre-launch refusal returns through qaFailure before reaching it: the --evidence read failure at doctor.go:352-363 (which prints evidence_field=/evidence_reason= and no stage) and the install-root failure at doctor.go:373 (error=actor_unavailable, reachable because qaInstallRoot fails when os.UserHomeDir fails, e.g. HOME unset). The new docs/operating.md:219 says "A qa run refusal names the preflight question that refused it (qa run refusals do not identify the failing stage and the route note misreports the resolved route #374)", and docs/operating.md:232 says an evidence refusal prints stage=evidence_admission with evidence_field/evidence_reason — the word order describes exactly the branch that prints the two fields without the stage. This is the ambiguity qa run refusals do not identify the failing stage and the route note misreports the resolved route #374 was filed for, still present in the two paths the doc promises to cover.
    • Failure scenario: cortex qa run --role requirements-analyst --request requirements.txt --catalog /abs/catalog --evidence missing.json prints error=invalid_request, the note=prerequisites line, then evidence_field=path evidence_reason=file_not_found, and exits failure with no stage= line, so the operator cannot tell this from any other pre-launch refusal; env -u HOME cortex qa run --role requirements-analyst --request requirements.txt --catalog /abs/catalog likewise prints error=actor_unavailable with no stage=actor_admission.
  • route_provider/route_model are written raw while the new doc claims they are escaped (internal/cli/doctor.go:408): fmt.Fprintf(stderr, "route_provider=%s route_model=%s\n", unavailable.Provider, unavailable.Model) writes the routed pair verbatim. That pair is the profile's own text: qaroute.routeText accepts any JSON string of up to 128 bytes with no charset restriction, resolve.go copies it into ResolvedRoute.Provider/Model, and qapi carries it into BackendUnavailableError (backend.go:70-83, report.go:172). The section this PR adds says the pair is "escaped for the terminal" (docs/operating.md:230), and the same PR sanitizes the caller-supplied --profile id through sanitizeProfileToken, so the escaping contract exists in the new surface but is not applied to the values the profile file controls. The model-probe test only feeds a benign pair (openai-codex/gpt-5.6-terra), so nothing catches it.
    • Failure scenario: A catalog whose cortex/qa-profiles.json routes the role to {"provider":"\u001b]52;c;SGVsbG8=\u0007","model":"m","effort":"low"} and a run whose availability probe answers model_unavailable writes the raw OSC 52 sequence into stderr on the route_provider= line (clipboard write / title rewrite in the operator's terminal), contradicting the documented escaping.

Suggestion (2)

  • Two of the three mapped route codes are unreachable and the classifier has no direct test (internal/cli/doctor.go:496): route_disallowed is emitted by qaroute.Resolve only when the role is absent from defaults or the backend is not admitted, and runQA already validates both before resolving (qarole.ValidateSquad at doctor.go:339, qaroute.RuntimeFor at doctor.go:333); non_nan_route is disclosed in the PR body as currently unreachable. The slice's tests exercise only profile_invalid and route_incomplete, so the mapping table itself is unpinned and a future code rename silently loses its stage. A table test over qaRefusalStage (or dropping the unreachable cases) would make the vocabulary evidence-backed rather than incidental.
  • Missing test conditions for the documented diagnostic fields (internal/cli/qa_test.go:655): Test conditions the new documentation promises but no test covers: (a) profile_source=unusable — a malformed or unreadable cortex/qa-profiles.json with --profile given, expected error=profile_invalid, stage=route_resolution, profile_source=unusable and no profile_valid_ids (doctor.go:540-542 is never exercised); (b) profile_selected=<id> echoed when --profile is passed (doctor.go:534) — the subtest passes --profile no-such-profile but asserts only profile_valid_ids, so the sanitization path is unverified; (c) profile_default=<id> is documented at docs/operating.md:204 and never asserted; (d) the --evidence missing.json path from the blocking finding; (e) the route resolution subtest's strings.Contains(stderr, "placeholder") guard can no longer fail for the intended reason because that token was removed from the note, so it is a vacuous assertion where an explicit !strings.Contains(stderr, "route_provider=") already carries the intent.

Question (1)

Verdict: not ready

@barbatdev

Copy link
Copy Markdown
Contributor Author

Fleet review (mimo)

Lens: requirements. Checked the diff against issue #374's four expected outcomes. Criterion 2 (the prerequisite note no longer claims the default route provider) is satisfied and pinned by a test. Criterion 3 (a refused --profile lists the valid ids, the selected id and the source state) is satisfied for the profile_invalid path and tested. Criterion 1 (refusals name the failing stage and stage input) is satisfied only for refusals that come out of RunLocalReportOn: two pre-pipeline refusal paths that belong to in-taxonomy stages still print no stage line — an unreadable evidence sidecar (internal/cli/doctor.go:349-364) and install-root resolution failure (internal/cli/doctor.go:371-374), contradicting both the criterion and this PR's own docs contract. Criterion 4 (doctor probe reporting distinguishes route resolution from actor binding from model probe) has no change in this diff; doctor's qa probe output (internal/cli/qabackend.go qaBackendLine) is untouched, and the PR's out-of-scope list does not mention it. The new docs/help promise that a qa run refusal names its preflight question, but request-file and cwd refusals print no stage and a pre-existing stage=catalog token sits outside the documented four-token vocabulary.

Blocking (2)

  • Evidence sidecar file-read refusal prints no stage line (internal/cli/doctor.go:352): The stage= line is only emitted in the RunLocalReportOn error branch (internal/cli/doctor.go:387, via qaRefusalStage). The earlier evidence-file read failure calls qaFailure and returns directly, printing evidence_field/evidence_reason (doctor.go:354-361) but no stage token, even though this PR's own documentation (docs/operating.md:232) defines stage=evidence_admission as 'the supplied evidence sidecar was refused before launch, with the evidence_field and evidence_reason that refused it'. That definition matches this path exactly, yet the stage is absent. No test covers stage naming for the evidence file-read path; the new test only covers in-pipeline schema validation.
    • Failure scenario: cortex qa run --role test-runner --request req.txt --catalog ./catalog --evidence ./missing.json prints 'error=invalid_request', the prerequisites note and 'evidence_field=path evidence_reason=file_not_found' with no stage= line at all, so issue qa run refusals do not identify the failing stage and the route note misreports the resolved route #374's expected outcome 'qa run refusals name the failing stage ... vs evidence admission' is not met for this input, and the output contradicts docs/operating.md:232.
  • actor_unavailable from install-root resolution prints no stage line (internal/cli/doctor.go:373): When skillroot.ResolveSystemUninstallRoots fails (HOME unset, or a documented PI_CODING_AGENT_DIR / CLAUDE_CONFIG_DIR override failing validation — resolve.go rejects the whole batch on one bad root), qaInstallRoot returns an error and runQA returns qaFailure(stderr, "actor_unavailable") directly: no qaRefusalStage call (doctor.go:387) and no admission_stage diagnostic (the qaAdmissionDiagnostic branch at doctor.go:401 never runs on this path). This is the same refusal code issue qa run refusals do not identify the failing stage and the route note misreports the resolved route #374 reports as unidentified, and it is untested for stage naming.
    • Failure scenario: With HOME unset (typical cron/service environment) or PI_CODING_AGENT_DIR='~/.pi/../x', 'cortex qa run --role test-runner --request req.txt --catalog ./catalog --backend pi' exits with 'error=actor_unavailable' plus the prerequisites note but no stage= line and no admission_stage=, so the actor-binding refusal remains unnamed — the exact defect issue qa run refusals do not identify the failing stage and the route note misreports the resolved route #374 asks to fix ('qa run refusals name the failing stage ... vs actor binding').

Question (2)

Verdict: not ready

@barbatdev

Copy link
Copy Markdown
Contributor Author

Post-merge QA (glm)

Slice 6/8: qa run refusals now name the preflight stage (route_resolution / actor_admission / model_probe / evidence_admission) via qaRefusalStage, the misleading default-provider claim is gone from the prerequisite note, and profile refusals print profile_selected (sanitized), profile_default, profile_valid_ids and route_setup. The four-stage vocabulary, the no-claimed-route rule for route refusals, the hostile-profile-id redaction and the post-launch-no-stage case are all pinned by tests, matching the linked issue's expected outcome. Nothing found that breaks what landed.

Suggestion (1)

  • profile_default and profile_valid_ids are printed without the same bounding as profile_selected (internal/cli/doctor.go:540): qaProfileDiagnostic sanitizes the caller-selected id with sanitizeProfileToken but prints profile_default and profile_valid_ids straight from qapi.ProfileSelection. They are safe only as long as the policy decoder admits just lowercase-word ids; if ProfileSelection ever returns ids the route policy would refuse (they are read from a caller-owned file), those values reach the terminal unsanitized while the selected id would have been redacted. Worth a defensive pass through the same sanitizer or a comment pointing at the decoder guarantee that makes it safe.

Verdict: clean

@barbatdev

Copy link
Copy Markdown
Contributor Author

Post-merge QA (deepseek)

Lens: evidence + test design. The slice delivers the stage vocabulary and the profile diagnostics #374 asked for: one fixed token per preflight question, no stage line for post-launch outcomes, the note no longer claims a route, and an unknown --profile now points at the file with its valid ids. The refusal tests are exact-string, which is the strongest form for this surface. Two evidence gaps remain: one diagnostic field skips the sanitizer the neighbouring field uses, and two codes in the stage switch are mapped but not driven by any test.

Suggestion (1)

  • Two codes in the stage switch are never driven through it (internal/cli/doctor.go:496): route_disallowed and non_nan_route map to route_resolution, but the PR body records non_nan_route as currently unreachable and no test passes either code through qaRefusalStage: TestQARunNamesThePreflightRefusalStage drives profile_invalid, route_incomplete, evidence, and the two typed errors. Test conditions: one case per code asserting stage=route_resolution, or drop the mappings so the vocabulary stays executable.

Question (1)

  • profile_default is printed without the sanitizer that guards profile_selected (internal/cli/doctor.go:538): qaProfileDiagnostic escapes the caller-supplied selection through sanitizeProfileToken (line 534) and then concatenates qapi.ProfileSelection's defaultID raw. The terminal guard is therefore only as strong as the weaker path: if ProfileSelection reports ok for a defaultProfile whose id carries bytes outside the sanitizer's [a-z0-9-] set (or if that validation later loosens), a caller-owned profile file puts control bytes on the operator's terminal through the one refusal path this slice built to be safe. What bounds defaultID today, and is the omission deliberate?

Verdict: clean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant