test(stand): scenario-design skills and agents, and a SCENARIOS.md coverage triage - #2323
Merged
Conversation
📝 WalkthroughWalkthroughAdded documentation for deployed-stand API and UI testing, scenario claim design, seeded persona reachability, invariant coverage, QA agents, and repository-derived quality-vector denominator counts. ChangesDeployed Stand QA
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
.claude/skills/playwright-cli/SKILL.md (1)
3-3: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse
npx --no-installfor the pinned Playwright CLI path.
src/frontenddeclaresplaywright@1.61.1, butnpx playwright teststill permits installing a different transientplaywrightpackage if it is not already present locally. Change this to require the lockfile version or usenpx --no-install playwright@1.61.1 test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/playwright-cli/SKILL.md at line 3, Update the Playwright CLI command guidance in the skill description to prevent transient package installation, using the repository’s pinned playwright@1.61.1 version via the lockfile or npx --no-install. Replace the unqualified “npx playwright test” reference while preserving the existing test-project guidance.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agents/stand-scenario-designer.md:
- Around line 3-5: Remove Bash from the tools list in the
stand-scenario-designer agent definition, leaving only Read, Glob, and Grep.
Preserve the agent’s read-only claim-design scope and all other frontmatter
unchanged.
In @.claude/agents/stand-test-auditor.md:
- Line 3: Update the description for the stand-test auditor to allow
metric-value comparisons in api/analytics/test_drilldown.py when they are
validated against independent evidence, while continuing to prohibit hardcoded
numeric metric expectations elsewhere.
- Line 85: Update the `## DO NOT` guidance in `stand-test-auditor.md` to add “Do
not” before “assume,” so the instruction explicitly requires verifying symbols
or routes with grep rather than assuming they exist.
In @.claude/skills/api-test/SKILL.md:
- Line 3: Update the api-test skill description for src/ingestion/tests/e2e/api/
to state that the in-process rig has authentication enabled, uses gateway JWTs,
and sets auth_disabled: false, matching api-auth-tenancy.md and api_coverage.py.
Keep the deployed stand description separate and unchanged as the
Keycloak-session and service-image environment.
In @.claude/skills/insight-stand/SKILL.md:
- Around line 255-259: Update the ledger descriptions in
.claude/skills/insight-stand/SKILL.md lines 255-259 and
.claude/skills/stand-ui-test/SKILL.md lines 207-210 to distinguish
PersonaSession constructing an ApiClient from UI journeys recording requests:
replace wording that says UI journeys never build a client with wording that
they do not call ApiClient.request, while preserving the API-only ledger
behavior.
- Around line 56-63: Update the playwright install command in
.claude/skills/insight-stand/SKILL.md lines 56-63 to include --frozen, and apply
the same command change in .claude/skills/stand-ui-test/SKILL.md lines 190-192.
Keep both commands under the tests project so browser installation uses the
locked dependency set.
- Around line 123-124: Replace the mutable
ghcr.io/constructorfabric/insight-ui-tests:latest image reference with the same
immutable published ref in the documented test commands across
tests/stand/README.md, .claude/skills/insight-stand/SKILL.md (lines 123-124),
and .claude/skills/stand-ui-test/SKILL.md (line 201). Keep the command behavior
unchanged apart from pinning the image consistently in all three files.
In @.claude/skills/stand-ui-test/SKILL.md:
- Around line 79-80: Update the URL guidance in the stand-UI test instructions
so localhost is required only for --image containerized runs or in-namespace
plain-HTTP runs. Allow secure HTTPS hostnames for remote HTTPS workflows,
preserving the existing __Host- cookie behavior guidance.
In `@tests/stand/api/test_gateway.py`:
- Around line 75-76: Align the route-wide docstring claim with the test
assertions in the parameterized operation sweep: either move the `/auth/login`
detail and context["reason"] == "no_session" checks into the per-operation
assertions, or narrow the docstring to describe only ALL_OPERATIONS[0]. Ensure
the documentation does not claim identical actionable refusal bodies for every
route unless those assertions run for every operation.
In `@tests/stand/README.md`:
- Around line 149-150: Update the gap-count statement in the README section to
match its four top-level bullets: change “Three gaps” to “Four gaps,” while
leaving the JWT verification bullet as a separate top-level item.
- Around line 163-166: Update the caveat in the README near the cluster-stand
explanation to accurately describe the implemented requires_seed behavior:
missing seed fixtures abort collection rather than skip tests. Do not claim a
skip path unless an explicit cross-tenant skip behavior is implemented and
tested.
In `@tests/stand/SCENARIO-COVERAGE.md`:
- Around line 103-113: Update the coverage-status labels for claims lacking
direct test evidence, including S1-I-01 and S1-I-02, from [VERIFIED] to
[SUPPORTED] or [INFERRED] as appropriate. Apply the same evidence-based labeling
to the cited visibility scope, R1-01/R1-02, R11-01/R11-02, R12, S1-I-03, and R6
entries, preserving [VERIFIED] only where the referenced evidence directly
proves the claim.
---
Nitpick comments:
In @.claude/skills/playwright-cli/SKILL.md:
- Line 3: Update the Playwright CLI command guidance in the skill description to
prevent transient package installation, using the repository’s pinned
playwright@1.61.1 version via the lockfile or npx --no-install. Replace the
unqualified “npx playwright test” reference while preserving the existing
test-project guidance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 724a5413-5220-49f8-a179-25a6a19226c7
📥 Commits
Reviewing files that changed from the base of the PR and between 2193228 and b67d729e973b6ad1d3549c99c08373a193f26b5b.
📒 Files selected for processing (19)
.claude/agents/stand-scenario-designer.md.claude/agents/stand-test-auditor.md.claude/skills/api-test/SKILL.md.claude/skills/insight-stand/SKILL.md.claude/skills/playwright-cli/SKILL.md.claude/skills/stand-api-test/SKILL.md.claude/skills/stand-scenarios/SKILL.md.claude/skills/stand-scenarios/invariants.md.claude/skills/stand-scenarios/persona-reach.md.claude/skills/stand-ui-test/SKILL.mdtests/lib/insight_stand/coverage.pytests/pyproject.tomltests/stand/README.mdtests/stand/SCENARIO-COVERAGE.mdtests/stand/api/conftest.pytests/stand/api/schemas/__init__.pytests/stand/api/test_gateway.pytests/stand/conftest.pytests/stand/meta/test_coverage_gate.py
aleksdotbar
approved these changes
Aug 7, 2026
ktursunov
marked this pull request as draft
August 7, 2026 07:35
ktursunov
force-pushed
the
test-plan-and-scenarios
branch
2 times, most recently
from
August 7, 2026 11:29
78a9d74 to
b3a8f0d
Compare
ktursunov
marked this pull request as ready for review
August 7, 2026 11:29
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/stand/SCENARIO-COVERAGE.md (2)
166-170: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep authorization stable in R9-01.
If role revocation changes visibility, the second read can return an error or an empty result. Equal fallback responses would not prove that historical figures stayed unchanged. Use a caller whose access remains stable, assert both reads return success, and compare the metric payload for the same past window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stand/SCENARIO-COVERAGE.md` around lines 166 - 170, Update the R9-01 scenario to use a caller whose authorization remains unchanged across the role revocation, assert both /v1/metric-results reads succeed, and compare their metric payloads for the same historical window; do not treat matching error or empty fallback responses as agreement.
136-140: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the S9-A-03 oracle prove universal scope.
The claim says every other viewer remains unchanged, but “two subchart reads” does not define a baseline or cover every non-grantee. Capture each relevant viewer’s reach before the grant, apply the grant, and compare the same viewers afterward.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stand/SCENARIO-COVERAGE.md` around lines 136 - 140, Update the S9-A-03 test coverage oracle to capture each relevant viewer’s reach before granting visibility, then apply the grant through /v1/visibility and read the same subcharts afterward. Compare every non-grantee’s post-grant reach with its baseline while separately verifying the grantee gains visibility, so the test proves the grant is scoped rather than universal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/quality-vector-tests/scripts/counts.sh:
- Around line 88-91: Update the "stand tests" count command in counts.sh to
replace the paste-and-bc summation pipeline with an awk-based sum, ensuring the
count still returns zero when no test definitions are found and does not depend
on paste.
In @.claude/skills/quality-vector-tests/SKILL.md:
- Around line 263-270: Update the statement near the catalog-count guidance to
clarify that no repo-wide denominator in this skill is authoritative, rather
than claiming the skill contains no numbers. Preserve the existing instruction
to re-run the counting script and its MOVED/--check behavior.
- Around line 69-71: Update every documented counts.sh invocation to use the
repository-root path .claude/skills/quality-vector-tests/scripts/counts.sh: in
.claude/skills/quality-vector-tests/SKILL.md at lines 69-71, 177-182, and
238-240, and in .claude/skills/quality-vector-tests/references/example-lean.md
at lines 8-11 and .claude/skills/quality-vector-tests/references/example-port.md
at lines 3-4. Replace the existing scripts/counts.sh and ../scripts/counts.sh
references without changing the surrounding workflow.
In @.claude/skills/stand-scenarios/invariants.md:
- Around line 157-187: Update R10 and R11 in the invariants documentation to
explicitly mark the threshold disagreement and missing S-2 conclusion-type
exclusion surface as unresolved. Align the corresponding entries in
tests/stand/SCENARIO-COVERAGE.md, and do not mark R10 complete until the chosen
product threshold is applied consistently to both frontend MIN_COHORT and
backend MIN_PEER_N enforcement.
---
Nitpick comments:
In `@tests/stand/SCENARIO-COVERAGE.md`:
- Around line 166-170: Update the R9-01 scenario to use a caller whose
authorization remains unchanged across the role revocation, assert both
/v1/metric-results reads succeed, and compare their metric payloads for the same
historical window; do not treat matching error or empty fallback responses as
agreement.
- Around line 136-140: Update the S9-A-03 test coverage oracle to capture each
relevant viewer’s reach before granting visibility, then apply the grant through
/v1/visibility and read the same subcharts afterward. Compare every
non-grantee’s post-grant reach with its baseline while separately verifying the
grantee gains visibility, so the test proves the grant is scoped rather than
universal.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e2b4a7d-8882-4cf9-8074-04502ed82aa6
📥 Commits
Reviewing files that changed from the base of the PR and between b67d729e973b6ad1d3549c99c08373a193f26b5b and b3a8f0db50658f4dc39d9fcfda9ccd802cfb4f96.
📒 Files selected for processing (7)
.claude/skills/quality-vector-tests/SKILL.md.claude/skills/quality-vector-tests/references/example-lean.md.claude/skills/quality-vector-tests/references/example-port.md.claude/skills/quality-vector-tests/scripts/counts.sh.claude/skills/stand-scenarios/SKILL.md.claude/skills/stand-scenarios/invariants.mdtests/stand/SCENARIO-COVERAGE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/stand-scenarios/SKILL.md
ktursunov
enabled auto-merge
August 10, 2026 01:24
ktursunov
disabled auto-merge
August 10, 2026 01:24
ktursunov
enabled auto-merge
August 10, 2026 01:25
Contributor
|
@ktursunov please answer / solve code rabbit comments |
Contributor
Author
|
Worked through all 16 review comments — 12 accepted, 3 declined (reasons on the individual threads), 1 recorded below rather than actioned. Applied in Two were real errors in this PR
Contradictions this PR introduced — the Usability bugs in its own additions — five of six Declined: the Not actioned, recorded here so it is not lost — the
This is a coverage gap that misreports itself as an environment fault; it is not evidence that tenant isolation is broken there. Left untouched deliberately: three authors describing a skip looks like unfinished wiring rather than a chosen behaviour, and editing the comments to match the code would cement it and erase the evidence of intent. Belongs to whoever owns that switch. |
ktursunov
force-pushed
the
test-plan-and-scenarios
branch
from
August 10, 2026 04:57
4e81a29 to
6c30469
Compare
ktursunov
force-pushed
the
test-plan-and-scenarios
branch
from
August 10, 2026 10:03
6c30469 to
5b89e0e
Compare
added 2 commits
August 10, 2026 18:08
…d suite
Turn docs/product/SCENARIOS.md into coverage for tests/stand/ without standing
up a parallel test-case database.
The product doc is already shaped for this: §1.1 Reach is a decision table
(4 personas x 7 dimensions with explicit "Never" cells) and §5 is twelve
numbered invariants. Its four personas map onto seeded fixtures, and the seed
enforces the distinction the doc cares about — the ceo holds the insight-admin
realm role but no admin row in person_roles, and admin_operator sits outside
the org chart entirely.
insight-stand operating the stand — up/seed/test/down, aiming a run
(base URL and manifest are two independent facts), session
fixtures, markers, mutation policy, triage
stand-scenarios SCENARIOS.md -> testable claims; five questions in order,
with persona-reach.md and invariants.md as the grounding
references, and SCENARIO-COVERAGE.md as the output
stand-api-test tests/stand/api/ — catalogue discipline, caller choice,
status codes, models, scratch policy, xfail policy, the gate
stand-ui-test tests/stand/ui/ — the UI-vs-API justification rule,
accessibility-first locators, page-object split, fidelity
stand-scenario-designer read-only; returns claims, writes no test code
stand-test-auditor adversarial; asks whether an assertion proves the
claim its name makes, and whether the test would
fail on an empty stand
SCENARIO-COVERAGE.md is the first pass: surface verdicts for S-1..S-10, the
clauses shipped tests already prove, 15 designed claims, and the gaps. Five of
ten scenarios produce claims; S-2, S-3 and S-6 have no surface. No claim
asserts a metric value — golden_metrics is empty by design.
Two findings worth a product decision are recorded there rather than acted on:
the group-size threshold is enforced server-side (MIN_PEER_N = 5) but the
frontend and the product doc both say four; and §5 rule 12's obvious test
cannot be requested, because a peer request carries only cohort_key and the
cohort is derived from the target rather than the request.
Documentation only — no product or test code changes.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…rrying stale numbers
Two pre-existing skills gained a same-named sibling in the previous commit and
needed the other half of a boundary that only ran one way.
`api-test` now says it is the IN-PROCESS rig and redirects deployed-stand work
to `stand-api-test`, stating why the route name cannot be the discriminator:
both suites exercise /v1/metric-results, /v1/queries and /v1/metric-drilldown,
and both have a per-operation coverage gate. It also no longer claims the rig
runs "with auth disabled" — `api-auth-tenancy.md` records `auth_disabled: false`
and a signed ES256 gateway JWT on every request.
`playwright-cli`'s description was thirteen unqualified words whose "work with
Playwright tests" clause matched "add a Playwright test" more literally than
`stand-ui-test` did. It now describes what the skill contains (the binary for
interactive driving, plus Node/TypeScript `npx playwright test`) and draws the
line by runner: this repository has no .spec.ts specs at all, and every
committed browser test is pytest under tests/stand/ui/.
`quality-vector-tests` carried denominators that had gone stale, and one of its
counting commands no longer worked at all: the catalog left `builtin.rs` for
`include_str!("registry.yaml")`, so the documented grep kept returning a
number — 0 — which reads exactly like an answer. A denominator of 0 in a target
is worse than no target.
So the counts are taken rather than quoted now. `scripts/counts.sh` prints each
denominator beside the path it came from, and every count proves its source
first: a moved source reports MOVED, never zero. `--check` exits non-zero for
CI. Verified against the real regression by pointing the catalog count back at
builtin.rs — it reports MOVED and --check exits 1.
The skill body carries no repo-wide number. Where a number is illustrative it
stays and is labelled a snapshot, because the worked examples teach the shape
of a finished target and that shape does not move.
Documentation only.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
force-pushed
the
test-plan-and-scenarios
branch
from
August 10, 2026 10:09
5b89e0e to
8ac8ee6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/insight-stand/SKILL.md:
- Line 243: Update the “Login loops, then 503” triage row to limit the diagnosis
to in-namespace plain-HTTP base URLs whose host is not localhost; do not imply
that remote HTTPS base URLs are invalid. Preserve the guidance about joining the
gateway network namespace or using the published port.
In @.claude/skills/quality-vector-tests/scripts/counts.sh:
- Line 62: Replace the `grep -q .` consumers in the source proofs at lines 62,
76, and 81 with a complete-consuming check such as `awk 'END { exit (NR == 0)
}'`. Update each affected pipeline so `find` is fully consumed under `pipefail`,
preserving the existing valid-source detection and `count()` behavior.
In @.claude/skills/stand-api-test/SKILL.md:
- Around line 243-250: Update the case-enumeration step in the Procedure to
require only statuses reachable from the specific handler: include all supported
validation outcomes such as 400 or 422, and add 404, 415, or scope/tenant
refusals only when the handler implements them. Remove the implication that
these statuses are universal while preserving one test per reachable status
code.
In @.claude/skills/stand-scenarios/SCENARIO-COVERAGE.md:
- Around line 169-173: Update the R9-01 scenario guidance to avoid revoking
roles in the shared seeded stand: require execution on an isolated stand with
complete state restoration, or mark the scenario blocked for tests/stand/.
Preserve the two metric-read temporal assertion while ensuring shared
person_roles state is never mutated.
- Around line 149-152: Move R1-01 out of the tests/stand/ scope and document it
under the value-capable metric suite owned by src/ingestion/tests/e2e/,
preserving its requirement to distinguish null from 0. Update the scenario
coverage entry and any implementation references accordingly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef1d91de-7229-4aed-b365-ac9b23ab879e
📥 Commits
Reviewing files that changed from the base of the PR and between b3a8f0db50658f4dc39d9fcfda9ccd802cfb4f96 and da9d5d6.
📒 Files selected for processing (12)
.claude/agents/stand-scenario-designer.md.claude/agents/stand-test-auditor.md.claude/skills/api-test/SKILL.md.claude/skills/insight-stand/SKILL.md.claude/skills/quality-vector-tests/SKILL.md.claude/skills/quality-vector-tests/references/example-lean.md.claude/skills/quality-vector-tests/references/example-port.md.claude/skills/quality-vector-tests/scripts/counts.sh.claude/skills/stand-api-test/SKILL.md.claude/skills/stand-scenarios/SCENARIO-COVERAGE.md.claude/skills/stand-scenarios/SKILL.md.claude/skills/stand-ui-test/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .claude/skills/quality-vector-tests/references/example-port.md
- .claude/skills/api-test/SKILL.md
- .claude/agents/stand-test-auditor.md
- .claude/skills/quality-vector-tests/references/example-lean.md
- .claude/agents/stand-scenario-designer.md
- .claude/skills/stand-scenarios/SKILL.md
| | `cannot resolve the stand's base URL — refusing to assume one` (lists what it tried) | no address from any source | `--base-url`, or run from the repo root where the env file lives | | ||
| | `gateway is not answering on http://localhost:<port>/` | you aimed `test-stand test` at a stand that is not local | the wrapper preflights localhost — run pytest directly instead | | ||
| | Everything skips with "capability … not present" | expected on this stand (`ingestion: no`) | not a failure — read the reason | | ||
| | Login loops, then 503 | **base URL is not `localhost`** — see the trap above | join the gateway netns, or use the published port | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit this diagnosis to plain HTTP.
Line 243 conflicts with Lines 108-112. A remote https:// base URL is valid. Only an in-namespace plain-HTTP URL that is not localhost causes the __Host- cookie failure. Update this triage row to state that condition.
Proposed wording
-| Login loops, then 503 | **base URL is not `localhost`** — see the trap above | join the gateway netns, or use the published port |
+| Login loops, then 503 | **in-namespace plain-HTTP base URL is not `localhost`** — see the trap above | use `localhost:<port>` for `--image` mode, or use a trustworthy HTTPS origin |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Login loops, then 503 | **base URL is not `localhost`** — see the trap above | join the gateway netns, or use the published port | | |
| | Login loops, then 503 | **in-namespace plain-HTTP base URL is not `localhost`** — see the trap above | use `localhost:<port>` for `--image` mode, or use a trustworthy HTTPS origin | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/insight-stand/SKILL.md at line 243, Update the “Login loops,
then 503” triage row to limit the diagnosis to in-namespace plain-HTTP base URLs
whose host is not localhost; do not imply that remote HTTPS base URLs are
invalid. Preserve the guidance about joining the gateway network namespace or
using the published port.
| count "connectors" \ | ||
| "src/ingestion/connectors" \ | ||
| "find src/ingestion/connectors -maxdepth 2 -mindepth 2 -type d | wc -l" \ | ||
| "find src/ingestion/connectors -maxdepth 2 -mindepth 2 -type d | grep -q ." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -o pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
for i in $(seq 1 10000); do
mkdir "$tmpdir/d$i"
done
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | grep -q .
bad_status=$?
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d |
awk 'END { exit (NR == 0) }'
good_status=$?
set -e
printf 'grep proof status: %s\n' "$bad_status"
printf 'awk proof status: %s\n' "$good_status"Repository: constructorfabric/insight
Length of output: 241
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf 'pwd=%s\n' "$(pwd)"
printf 'counts_file_exists=%s\n' "$(test -f .claude/skills/quality-vector-tests/scripts/counts.sh && echo yes || echo no)"
if test -f .claude/skills/quality-vector-tests/scripts/counts.sh; then
wc -l .claude/skills/quality-vector-tests/scripts/counts.sh
sed -n '1,100p' .claude/skills/quality-vector-tests/scripts/counts.sh | cat -n
fi
printf '\nBash version:\n'
bash --version | head -n 1
printf '\nBehavioral probe 1: early grep under pipefail with large find output:\n'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
dirs=()
for i in $(seq 1 "$((256 * 1000))"); do
dirs+=("d$i")
done
mkdir -p "$tmpdir"/"${dirs[@]}"
set +e
find "$tmpdir" -min RRepository: constructorfabric/insight
Length of output: 5054
🏁 Script executed:
#!/usr/bin/env bash
set -u
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
printf '\nBehavioral probe 1: early grep under pipefail with large find output:\n'
for i in $(seq 1 "$((256 * 1000))"); do
mkdir "$tmpdir/d$i"
done
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | grep -q .
grep_status=$?
find_exit_status=$?
set -e
printf 'grep -q exit status: %s\n' "$grep_status"
printf 'find exit status in pipeline with bash $? after set +e: %s\n' "$find_exit_status"
printf '\nBehavioral probe 2: small find output under pipefail:\n'
for i in $(seq 1 2); do
mkdir "$tmpdir/small$i"
done
set +e
find "$tmpdir/small" -mindepth 1 -maxdepth 1 -type d | grep -q .
grep_small_status=$?
set -e
printf 'grep -q small output exit status: %s\n' "$grep_small_status"
printf '\nBehavioral probe 3: complete consumer under pipefail with large find output:\n'
for i in $(seq 1 "$((256 * 1000))"); do
mkdir "$tmpdir/compl$i"
done
set +e
find "$tmpdir/compl" -mindepth 1 -maxdepth 1 -type d | awk 'END { exit (NR == 0) }'
awk_status=$?
set -e
printf 'awk complete consumer exit status: %s\n' "$awk_status"Repository: constructorfabric/insight
Length of output: 756
🏁 Script executed:
#!/usr/bin/env bash
set -u
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
printf '\nBehavioral probe 1: early grep under pipefail with large find output:\n'
for i in $(seq 1 "$((256 * 1000))"); do
mkdir "$tmpdir/d$i"
done
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | grep -q .
grep_status=$?
set -e
printf 'grep -q exit status: %s\n' "$grep_status"
printf '\nBehavioral probe 2: small find output under pipefail:\n'
for i in $(seq 1 2); do
mkdir "$tmpdir/small$i"
done
set +e
find "$tmpdir/small" -mindepth 1 -maxdepth 1 -type d | grep -q .
grep_small_status=$?
set -e
printf 'grep -q small output exit status: %s\n' "$grep_small_status"
printf '\nBehavioral probe 3: complete consumer under pipefail with large find output:\n'
for i in $(seq 1 "$((256 * 1000))"); do
mkdir "$tmpdir/compl$i"
done
set +e
find "$tmpdir/compl" -mindepth 1 -maxdepth 1 -type d | awk 'END { exit (NR == 0) }'
awk_status=$?
set -e
printf 'awk complete consumer exit status: %s\n' "$awk_status"Repository: constructorfabric/insight
Length of output: 698
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf 'Bash version:\n'
bash --version | head -n 1
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
printf '\nProbe: large find output with early-close consumer under pipefail:\n'
dirs=()
i=0
while true; do
dirs+=("d$i")
i=$((i + 1))
if [ "$i" -ge 60000 ]; then break; fi
done
mkdir -p "$tmpdir"/"${dirs[@]}"
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | grep -q .
early_status=$?
set -e
printf 'early-close status: %s\npython3 status: %s\n' "$early_status" "$(python3 -c 'print(0 if early_status == 0 else 69)')"
printf '\nProbe: large find output with complete consumer under pipefail:\n'
dirs=()
i=0
while true; do
dirs+=("c$i")
i=$((i + 1))
if [ "$i" -ge 60000 ]; then break; fi
done
mkdir -p "$tmpdir"/"${dirs[@]}"
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | awk 'END { exit (NR == 0) }'
complete_status=$?
set -e
printf 'complete status: %s\n' "$complete_status"Repository: constructorfabric/insight
Length of output: 550
🏁 Script executed:
#!/usr/bin/env bash
set -u
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
printf '%s directories\n' 60000
for i in $(seq -w 1 60000); do
mkdir "$tmpdir/$i"
done
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | grep -q .
early_status=$?
find_status=$?
set -e
printf 'pipefail behavior: grep status=%s find pipeline status=%s\n' "$early_status" "$find_status"
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | grep -vq .
awk_status=$?
set -e
printf 'grep-vq empty behavior: status=%s\n' "$awk_status"
set +e
find "$tmpdir" -mindepth 1 -maxdepth 1 -type d | awk 'END { exit (NR == 0) }'
awk_status=$?
set -e
printf 'awk complete consumer behavior: status=%s\n' "$awk_status"Repository: constructorfabric/insight
Length of output: 348
🏁 Script executed:
#!/usr/bin/env bash
set -u
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
mkdir -p "$tmpdir/60000"
set +e
find "$tmpdir" -min RRepository: constructorfabric/insight
Length of output: 194
🌐 Web query:
GNU find mindepth predicate syntax find -mindepth
💡 Result:
The GNU find command uses the -mindepth predicate to restrict the depth at which tests and actions are applied [1][2]. Syntax: find [path...] -mindepth levels The levels argument must be a non-negative integer [1][2]. When -mindepth is used, the command will not apply any tests or actions to files at levels less than the specified integer [3][4]. Key behavioral notes: -mindepth 0 is the default behavior, meaning it processes the starting-point(s) as well as all files and directories found below them. -mindepth 1 prevents the command from applying tests or actions to the starting-point(s) themselves, processing only the contents within them [1][3]. Example: To find files at least 2 levels below the directory 'dir', you would use: find dir -mindepth 2 This will skip 'dir' and 'dir/subdir' (level 0 and 1) and output files starting from 'dir/subdir/file' [4].
Citations:
- 1: https://manpages.debian.org/bullseye/findutils/find.1.en.html
- 2: https://www.man7.org/linux/man-pages/man1/find.1.html
- 3: http://gnu.ist.utl.pt/software/findutils/manual/html_mono/find.html
- 4: https://web.archive.org/web/20220511203639/http:/www.gnu.org/software/findutils/manual/html_mono/find.html
Avoid early-closing source proofs under pipefail.
Lines 62, 76, and 81 use grep -q . as the source proof after set -o pipefail. grep -q can stop after the first match and close the pipe while find is still writing, causing the proof to fail and count() to report a valid source as MOVED.
Use a complete consumer for the proof, such as find ... | awk 'END { exit (NR == 0) }', and apply it to all source proofs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/quality-vector-tests/scripts/counts.sh at line 62, Replace
the `grep -q .` consumers in the source proofs at lines 62, 76, and 81 with a
complete-consuming check such as `awk 'END { exit (NR == 0) }'`. Update each
affected pipeline so `find` is fully consumed under `pipefail`, preserving the
existing valid-source detection and `count()` behavior.
Comment on lines
+243
to
+250
| ## Procedure | ||
|
|
||
| 1. Read the handler in `src/backend/services/<service>/src/api/`. Declared codes | ||
| are boilerplate; reachable codes are in the code. | ||
| 2. Add the operation to `operations.py` if it is new. | ||
| 3. Pick the caller — the choice is usually the test. | ||
| 4. Enumerate cases: success, each validation 400, path-parse 400, 404 unknown, | ||
| 415, and the scope/tenant refusals. One test per code. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe only reachable status cases.
Line [136] allows off-schema responses of 400 or 422, but Line [249] lists only validation 400. The procedure also presents 404, 415, and scope refusals as universal cases.
Update the step to include each reachable status, such as 400 or 422, and add 404, 415, or scope cases only when the handler supports them.
Suggested wording
-4. Enumerate cases: success, each validation 400, path-parse 400, 404 unknown,
- 415, and the scope/tenant refusals. One test per code.
+4. Enumerate success and each reachable validation status, such as 400 or
+ 422, path-parse 400, unknown-resource 404, 415, and scope/tenant refusals.
+ Add one test per reachable status.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/stand-api-test/SKILL.md around lines 243 - 250, Update the
case-enumeration step in the Procedure to require only statuses reachable from
the specific handler: include all supported validation outcomes such as 400 or
422, and add 404, 415, or scope/tenant refusals only when the handler implements
them. Remove the implication that these statuses are universal while preserving
one test per reachable status code.
Comment on lines
+149
to
+152
| **R1-01 — an unmeasured metric answers `null`, never `0`.** The UI half is | ||
| covered; the API half is not, and the two are indistinguishable on screen — a | ||
| backend returning `0` and a frontend rendering `—` for falsy look identical. | ||
| API `POST /v1/metric-results` · P1 · new · `[VERIFIED]` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Move R1-01 to the value-capable suite.
R1-01 must distinguish null from 0. This is a metric-value oracle. .claude/skills/stand-api-test/SKILL.md forbids this assertion because golden_metrics is empty. The same file identifies src/ingestion/tests/e2e/ as the owner for metric-value coverage.
Mark R1-01 out of scope for tests/stand/, or move its implementation to the value-capable suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/stand-scenarios/SCENARIO-COVERAGE.md around lines 149 - 152,
Move R1-01 out of the tests/stand/ scope and document it under the value-capable
metric suite owned by src/ingestion/tests/e2e/, preserving its requirement to
distinguish null from 0. Update the scenario coverage entry and any
implementation references accordingly.
Comment on lines
+169
to
+173
| **R9-01 — revoking a role does not change a past period's figures.** Needs no | ||
| value oracle: read the same past window twice, either side of the revoke, and | ||
| assert the two reads **agree**. That is the whole claim, and it is the temporal | ||
| guarantee the doc leans on in three separate scenarios. API `/v1/person-roles` + | ||
| two `/v1/metric-results` reads · P1 · new · `[SUPPORTED]` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not revoke seeded roles in the shared stand.
R9-01 requires revoking a role between two metric reads. The stand API skill defines the deployed stand as read-only and permits mutation only for scratch resources created and deleted through the API.
Run this claim on an isolated stand with full state restoration, or mark it blocked for tests/stand/. Otherwise, the test can change person_roles state for later tests and later runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/stand-scenarios/SCENARIO-COVERAGE.md around lines 169 - 173,
Update the R9-01 scenario guidance to avoid revoking roles in the shared seeded
stand: require execution on an isolated stand with complete state restoration,
or mark the scenario blocked for tests/stand/. Preserve the two metric-read
temporal assertion while ensuring shared person_roles state is never mutated.
aleksdotbar
approved these changes
Aug 10, 2026
This was referenced Aug 11, 2026
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
3 participants
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.
Tooling to turn
docs/product/SCENARIOS.mdinto coverage for the deployed-stand suite (tests/stand/), plus a first triage pass. Relates to #2188 ([research] quality vectors) and feeds the gate program in #1404.No product code changes. Four skills, two agents, one derived artifact, and corrections to suite comments the tooling was written from.
Why this shape
The product doc turns out to be unusually testable. §1.1 Reach is a decision table — 4 personas × 7 dimensions with explicit Never cells — and §5 is twelve numbered invariants. Those are assertions someone already wrote down. Its four personas also map cleanly onto seeded fixtures, and the seed enforces the distinction the doc cares about: the
ceoholds theinsight-adminrealm role but noadminrow inperson_roles, so seniority is refused byrequire_admin, whileadmin_operatorsits outside the org chart entirely.Deliberately not a local test-case management system — no case bodies, no execution status, no per-run results.
What's here
insight-standstand-scenariospersona-reach.mdandinvariants.mdas grounding referencesstand-api-testtests/stand/api/— catalogue discipline, caller choice, status codes, models, scratch policy, the coverage gatestand-ui-testtests/stand/ui/— the UI-vs-API justification rule, locators, page-object split, fidelityAgents:
stand-scenario-designer(read-only, returns claims) andstand-test-auditor(adversarial — would this fail for the right reason?).tests/stand/SCENARIO-COVERAGE.mdis the first pass: surface verdicts for S-1…S-10, ten clauses the shipped tests already prove without naming them, 15 designed claims, and the gaps. Five of ten scenarios produce claims; S-2, S-3 and S-6 have no surface and that verdict is a deliverable, not a failure. No claim asserts a metric value —golden_metricsis empty by design.Findings worth a decision
Two group-size thresholds are enforced, and they disagree. §5 rule 10 says a group figure is not shown below four people.
MIN_PEER_N = 5is compiled into the peer SQL server-side (analytics/src/domain/metric_results/compiler.rs:25);MIN_COHORT = 4is client-side (within-team-peer.ts:17). A cohort of exactly four measured members sees a median on screen that the API declines to compute. Appendix C already lists the threshold as needing confirmation — this is the concrete version of that question.§5 rule 12's obvious test cannot be written. "The same cohort at team scope versus organization scope" can't be requested: a peer request carries only
cohort_key, all 59 definitions declareorg_unit, and the cohort CTE selects on the target'scohort_idindependently ofentity.ids. A narrower claim survives.Appendix C's first open point is answerable now — ADMIN gaining no implicit data visibility is enforced at the API and already tested.
Suite corrections
The skills were drafted from suite comments, several of which were wrong — so those are fixed here too rather than left to mislead the next reader:
pytest_sessionfinishclaimed browser journeys record into the coverage ledger.coverage.recordhas one call site (ApiClient.request) and noui/module builds a client, so a ui-only run produces neither ledger nor catalogue and the gate cannot run on that pair.200for 19 operations, not 18; analytics has no/v1/admin/*route;requires_catalogueadvertised atable_columnspart that isn't implemented.api-testandplaywright-clidescriptions gained the reciprocal half of a boundary that only ran one way. Both suites exercise/v1/metric-resultsand both have a coverage gate, so the route name never disambiguated them; and this repo has no.spec.tsat all, so "add a Playwright test" always means the pytest journeys.Review notes
ruffclean.Summary by CodeRabbit
New Features
Documentation