Add bounded CwdMode observability to WhichResolver, and repair the CodeScene report hand-off (#718) - #748
Draft
leynos wants to merge 22 commits into
Draft
Add bounded CwdMode observability to WhichResolver, and repair the CodeScene report hand-off (#718)#748leynos wants to merge 22 commits into
leynos wants to merge 22 commits into
Conversation
added 10 commits
September 19, 2026 17:10
`WhichResolver` searches four domains — PATH only, PATH plus the workspace root, PATH plus the current directory, and PATH followed by a recursive workspace walk — but its counters recorded only the cache outcome, the final result, and the error category. A `workspace-recursive` miss and an `auto` miss produced the identical series, so an operator could not tell whether recursive lookup contributed to a resolution or whether a manifest had requested it at all. Move both resolver counters into a `telemetry` module that owns their names and every label vocabulary, and add a `cwd_mode` label drawn from a closed four-value set. The span carries the same value at creation, so a trace correlates with the series it produced. The label is the bounded telemetry spelling, not the template spelling: the module maps `CwdMode::WorkspaceRecursive` to `workspace_recursive`, and the existing `category` vocabulary is now single-sourced there rather than repeated as string literals. Nothing a template supplies reaches a label: no command name, path, workspace name, or environment value is recorded, so the series stay exportable. The application recorder admits each series by its exact label shape. The resolution counter is admitted under two shapes because a failure adds `category` and a success does not. The tracing capture helper now records span fields by span name, since a field set at creation never reaches `on_record`; the previous discovery-span-only accessor is generalised rather than duplicated.
Adds resolver-side and application-recorder coverage for the `cwd_mode` label. The resolver suite drives the real `WhichResolver` through a local `DebuggingRecorder`, so the series asserted are the ones a manifest produces; the tracing cases install a temporary subscriber and check the span fields and the failure event field by field. Two properties are pinned. The `cwd_mode` label separates resolutions that are otherwise identical, which is what the counters could not do before. And nothing outside the closed vocabularies leaves the process: the command name, the workspace root, and the matched path are asserted absent from every captured event and span field. The recorder cases are the allowlist's specification. They exposed an over-permissive rule: the three-label shape admitted any declared category on any outcome, so a `found` series carrying a `category` was exported even though no call site can produce one. `WHICH_RESOLUTION_FAILURE_OUTCOME_VALUES` now names the two outcomes that legitimately carry a category, and the three-label shape admits only those. The tracing capture helper gained a named-span accessor because the resolver span is not the discovery span; `span_fields` keeps the existing discovery-only call site working unchanged.
The trunk lane wrote `lcov.info`, read it back through an input of its own, and uploaded it, and nothing in between ever opened the file. Existence was the whole check, so an empty or truncated report -- which the generation action reports success for -- reached CodeScene and was refused there, hours later and in another system, as a check run reading "No valid coverage report found in the build pipeline" against a commit whose job passed every step. That message names neither the step nor the file at fault. The lane now stages the report into a directory of its own and runs `scripts/validate_coverage_artifact.py` over it. That validator already owns the LCOV contract for a hostile report, reads the file as data without executing anything in it, and is exercised by `make test-coverage-artifact`; it had no live subject until now. The step sits after the report is written, before the upload that sends it, and before `Show sccache statistics`, which `sccache_contract_test.py` requires to follow every compile step. The new `codescene_upload_invariants` predicates hold the lane to the rest of the delivery contract, and `codescene_upload_contract_test` drives them against the repository file and against synthetic lanes, so a detector that stopped matching cannot let the assertion pass over an empty set. They cover the ordering of the three steps, the input names the generator and the upload agree on, the format they agree on, the credential being both carried and gated on, the generator's archive surviving, and the checksum input that is no longer passed: its value came from `vars.CODESCENE_CLI_SHA256`, and this repository declares no variables, so it resolved to the empty string and verified nothing while reading as though it did. `workflow_variable_scan` holds the `vars.` scan those predicates use. It is general to any step rather than particular to this lane, so it lives apart from the lane's own contract, as does its test module.
Two documents gained contracts this branch introduced. ADR-024 carries the resolver's bounded search-domain telemetry as an addendum: the `cwd_mode` label and its closed set, the two counters and the span that carry it, the note that the vocabulary is a telemetry spelling rather than the template spelling, the deliberate series-shape compatibility change, and the redaction rules that are unchanged. The developers guide and the design document describe the same contract where each already discusses the `which` domain. ADR-025 carries the report-delivery contract the trunk lane now enforces. Its `## Verification` section gains the reason the lane reads the report as data before submitting it, the validator it runs, the step's required position, and the workflow contract test that holds the lane to it. Two sentences in that ADR and in the developers guide described the hostile-artefact validators as standalone maintenance tools; they no longer are, because the trunk lane runs the outer one over the report it generated itself. Both sentences now say so without disturbing the claim they were making, which is that no active workflow downloads pull-request coverage.
`make lint` reported two Whitaker findings against the branch-new test module, both of which the suite is right about. `module_max_lines` saw 591 lines against a 400-line budget. The cases split cleanly by what they read: the counter series in one child, the span and its event in the other. Both are declared with an explicit `#[path]`, following the layout `ninja_gen_tests.rs` and `ninja_gen_property_tests.rs` already use, so the implicit same-stem rule does not fire on the new directory. `no_expect_outside_tests` saw `NonZeroUsize::new(8).expect(..)` in `Workspace::resolver`. That helper arranges state rather than asserting, and arrangement can fail, so it now returns `Result` and each caller propagates with `?`. The capacity is a literal, so the error branch is unreachable, but the house policy is that only a test body may unwrap — a fixture makes the failure a value and lets its caller decide. The public consts' doc comments also lose two intra-doc links. `CwdMode` and `ResolveError` are `pub(crate)`, and `Cargo.toml` denies `private_intra_doc_links`, so `cargo doc` refused the links from the public `WHICH_CWD_MODE_VALUES` and `RESOLVE_ERROR_CATEGORY_VALUES`. Plain code spans carry the same meaning without widening the public API to keep a link alive.
`make typecheck` reported ten `invalid-argument-type` diagnostics against the
branch-new contract suites, all of the same two shapes.
`step_named` returns `dict | None`, and the guard was `any(step is None for
step in ordered)`. That predicate is true or false as a whole, so the calls
that follow could not narrow each name to a step. Spelling it as three
identity tests restores the narrowing without changing when it triggers.
`unbound_variable_references` took a `dict[str, object]`, which is invariant
in its value type: the test's `{"if": UNDECLARED}` infers `dict[str, str]` and
so was rejected even though the scan only reads. A `Mapping` is the honest
parameter — covariant, and all the function needs.
`make test` failed `span_fields_are_captured_by_name_and_recording_point`: the span declared `later` only at `record` time, and `Span::record` resolves a field name against the span's declared set, so the call was dropped before it could reach `on_record`. The test therefore never exercised the recording point it exists to cover, and its expected value was unreachable. Declaring `later = tracing::field::Empty` at creation makes the `record` resolve, which is the shape the passing sibling case already uses. `make lint` failed `PLR0916` on the three-way `is None` guard added when `ty` rejected the original `any(...)` predicate. The two constraints are satisfiable together: `report_steps` now loops over one name at a time and returns early, so the guard is a single test and the returned tuple carries three narrowed steps. The absent-step message reads the names back from the same const the lookup walks, so the two cannot drift.
Adopt the action-reference convention main introduced in #731: a contract asserts the action a step names and the shape of its pin, never the revision, because the revision is whatever the dependency updater last wrote. `codescene_upload_invariants` now routes both references through the shared checker, and the clean-lane fixture carries a full SHA so the shape rule is exercised rather than assumed. Five negative cases cover a tag, a branch, an abbreviated SHA, an uppercase SHA, and a bare path. Extracting the two lane-generic helpers, `step_named` and `action_reference_of`, keeps both modules inside the 400-line ceiling the Python lint gate enforces. The invariants module had reached exactly 400 lines, so the pin work could not land without the split. `action_reference_of` turns the shared checker's assertion into an offender string, so one bad reference joins the rest of the report rather than ending the scan.
`make test-workflow-contracts` was described in the developer guide but absent from both the "Quality gates" list and `AGENTS.md`'s pre-commit list. `make test` runs the Rust suite only and `make lint` lints the Python sources without executing them, so a change to a workflow or to a suite under `tests/workflow_contracts/` was verified by no documented gate at all: the contract could be edited into a shape it no longer enforces, and every listed command would still pass. Add it to both lists, conditional on the change touching a workflow, a workflow-contract suite, or the coverage artefact validators under `scripts/`, and state why neither of the two standing gates covers those suites. Fold the duplicated "`make test` runs only the Rust suite" sentence in the guide into the paragraph that now makes the same point once.
The step's comment said `uv` was on PATH because the generation action "also exports UV_PYTHON_INSTALL_DIR". The action does install `uv` — its first steps run `astral-sh/setup-uv` and then `uv run` throughout, which the comment now says — but it never sets that variable: `setup-uv`'s `python-version` input sets `UV_PYTHON`, and the shared action does not pass the input at all. Nothing in this repository references the variable. State the verified reason instead. The header also asserted that the generation action reports success for an "empty or truncated" report. What is verifiable is narrower and is what the step actually guards: the upload asserts only that the file exists and then hands it to `cs-coverage upload`, so any malformed report reaches CodeScene and is refused there. Say that. Both are comment-only. The shell body is unchanged, and the workflow contracts, actionlint, yamllint, check-fmt, and markdownlint all pass over the edited file.
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Contributor
Reviewer's GuideThe PR independently repairs CodeScene report delivery by validating the trunk LCOV artifact before upload and enforcing the workflow contract, and adds redacted, bounded cwd_mode telemetry to WhichResolver while preserving search semantics. Sequence diagram for bounded WhichResolver telemetrysequenceDiagram
participant Caller
participant Resolver as WhichResolver
participant Telemetry
participant Recorder
Caller->>Resolver: resolve(command, options)
Resolver->>Telemetry: cwd_mode_label(options.cwd_mode)
Resolver->>Telemetry: record_cache_outcome(cwd_mode, outcome)
Telemetry->>Recorder: Export bounded cache labels
Resolver->>Telemetry: record_resolution_found(cwd_mode)
Telemetry->>Recorder: Export bounded resolution labels
Resolver->>Telemetry: record_resolution_error(cwd_mode, error)
Telemetry->>Recorder: Export bounded failure labels
Flow diagram for validated CodeScene coverage publicationflowchart LR
Generate[Generate lcov.info] --> Stage[Stage lcov.info in dedicated directory]
Stage --> Validate[validate_coverage_artifact.py]
Validate -->|valid| Upload[CodeScene upload]
Validate -->|invalid| Fail[Fail workflow early]
Upload --> Stats[Show sccache statistics]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The scan that forbids undefined repository variables located a reference
only when it immediately followed `${{`. A compound condition such as
`github.event_name == 'push' && vars.SECRET != ''`, a call such as
`contains(vars.FOO, 'x')`, and a second expression in the same value all
reported clean while resolving to the empty string — the exact failure the
scan exists to prevent, in the shapes most likely to be written by hand.
Locate the `${{ ... }}` regions first and scan each body second, so a
reference is found at any position inside an expression while text that
merely spells `vars.` outside one stays unreported.
Three tests pin the properties that generality rests on: a reference is
found in a conjunction, a function argument, and a later expression; a
literal spelling is not a reference; and a value naming several variables
is reported once, since the caller reports which values to inspect.
Also correct two prose defects the same review raised. The pre-commit
target list claimed `make test-workflow-contracts` covered changes to the
coverage artefact validators under `scripts/`, which it does not run —
`make test-coverage-artifact` owns those, and neither target runs the
other. And the sccache ordering constraint in ADR-025 and the developer
guide read as a description of the lane rather than a requirement
`tests/workflow_contracts/sccache_contract_test.py` imposes on it.
Two ruff defects in the same file are fixed with it: a manual list
comprehension, and a docstring past the configured line limit.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Four review findings, each verified by probe before the fix and each fix falsified afterwards to show it is load-bearing. The workflow expression scan stopped at a newline. A YAML literal block keeps its newlines after parsing, so a step that breaks an expression across two lines is scanned as text containing one, and `.` without `DOTALL` never closed the region: a `vars.` reference inside went unreported. The scan now reads across newlines. The upload's credential gate was matched by substring, and `CS_ACCESS_TOKEN` is a substring of `NOT_CS_ACCESS_TOKEN`. All three checks accepted a gate on the longer, unset name, which compares '' with '' and never opens, so the lane would read as gated while submitting nothing. The scan now enumerates `(namespace, name)` identifier pairs — the unit a GitHub Actions expression addresses a value with — and the gate is held to the exact name in the `env` namespace the `if` is evaluated against. Since GitHub evaluates `if` as a bare expression, the extractor takes a `bare` keyword that scans the whole value; the delimited form is what the rest of the workflow uses. The validation-step predicate required the validator script and an `--artifact-dir` argument, and stopped there. Naming a staged directory without copying the report into it validates whatever else is in it — on a runner, nothing. The predicate now requires the copy as well, and the contract test gains a case for a stage that is never filled. The recorder admitted two-label resolution series whose outcome was any of `found`, `not_found`, or `error`, so a failure recorded without its category matched the success shape and reached the snapshot. `WHICH_RESOLUTION_SUCCESS_OUTCOME_VALUES` names the one outcome the two-label shape may carry, making the two vocabularies disjoint complements: a `found` series with a category and a failure without one are both refused. Co-Authored-By: Claude Code <noreply@anthropic.com>
Three findings from `make lint`, all in files this branch adds: an unparenthesised implicit string concatenation in the contract test's table, an `f` prefix on a regex with no placeholders, and a missing Returns section on `_copies_report_into`. Co-Authored-By: Claude Code <noreply@anthropic.com>
`make lint` failed on `too-many-lines`: the upload contract test had grown to 403 lines and its invariants module to 489, against a cap of 400 that pytest, ruff and pylint all enforce. Split along the seams the suite already had. The rules about the secret the lane is handed — which namespace it is read from, that it is read from a secret, and that the step is gated on it by identifier rather than by a substring of the text around it — describe a credential, not a report, so they move to `codescene_credential_invariants`. The synthetic lane the contract test drives its cases on, its fixtures, and the accessors it reads them back through are data rather than contracts, so they move to `codescene_upload_lane_data`, following the `sccache_compile_step_data` and `cache_contract_data` precedent. The suite is unchanged at 606 passed, 2 skipped. Co-Authored-By: Claude Code <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The second review pass found five shapes the report-delivery contract
examined in appearance but not in substance.
A declared value of `env.secrets.CS_ACCESS_TOKEN` contains `secrets.`
while reading the field off the step's own environment: the credential
came from `env`, not from the secret store, and the substring test
accepted it.
A gate spelling the credential inside a string literal gated nothing.
GitHub's expression grammar treats a name as a reference only when it is
written unquoted, so `${{ 'env.CS_ACCESS_TOKEN' != '' }}` compares a
non-empty literal against the empty string, is always true, and never
opens on anything. `expression_references` now strips single-quoted
literals before enumerating identifiers, which is the separator the
grammar itself uses.
`step_named` returned the first of a repeated name and its docstring
claimed a caller asserted uniqueness, which none did. GitHub keys nothing
on a step's name, so a copy-pasted step keeps its original name and runs:
two uploads, the second unpinned, in a lane the contract declared clean.
`step_names_declared_twice` answers that as the fault it is, the lookup
stays a lookup, and `report_steps` refuses a lane that repeats one of the
three names rather than reporting claims about an arbitrary member.
`_copies_report_into` matched `mv`, which satisfies it while removing the
workspace copy the upload has yet to read. Only `cp` and `install` are
copying commands; the pattern is now anchored to them.
The fixture resolver pinned `PATH` but not `PATHEXT`. On Windows the
fixture executable is written as `<command>.cmd` and the override is what
tells the resolver the extension is executable, so a host whose `PATHEXT`
omitted `.CMD` would fail the hit cases on a correct resolver.
The report-validation predicates moved to their own module: the additions
took `codescene_upload_invariants` past the 400-line cap, and the third
bullet's whole remedy is stated over the validating step alone.
Eight cases pin the fixes, including the two containment shapes and the
`mv`-as-copy shape that previously passed.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
CodeScene scored `Series::matches` as a Complex Method at impact 9.47. The method answered four questions in one expression, three of them through a closure that rescanned the label list between each test, and the fourth through a `map_or_else` whose two arms read as different kinds of question. `LabelSet` now owns the label scans, so each predicate asks once what it is about. `Series` keeps one method per clause — the counter's identity, its category, its count — and `matches` composes them, which is what the CodeScene rule is asking for rather than a shorter expression. `assert_cache_counter` had the same repeated-rescan shape and now reads through the same type. Behaviour is unchanged: the three series cases pass, and the label-count assertion that a bounded pair is a set of two is still made at the same place.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
`is_resolution_counter` reads nothing from `self`, so clippy's `unused_self` fired on it once the extracted helper was compiled. The question it answers is about the entry — is this a counter under the resolution metric's own name — rather than about any expectation, so an associated function is what it should have been. The two sibling predicates genuinely read `self` and stay methods. Gate evidence at this content: make lint (all four sub-lints, including lint-whitaker and lint-python), check-fmt, typecheck, markdownlint, nixie, test-workflow-contracts (614 passed, 2 skipped), test-coverage-artifact, doc-coverage, github-actions-lint — all green.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The report-delivery contract's two remaining clauses were stated over lines of shell, which is not what a shell runs. A one-liner joining several commands with `&&` therefore read as one: a `cp` of some unrelated file satisfied the copy rule when a later command on the same line named the staged directory, and a directory that was never created satisfied the staging rule because the argument only had to be non-empty. Both predicates now read the script as commands. `_command_segments` splits on `&&`, `||`, `;` and `|`, and each check is asked of one segment at a time; the copy has to name both the report and the directory in the same command, and the directory has to be one the script creates — through `mktemp --directory`, in either spelling, or through `mkdir`, with the variable or the literal matching what the flag was handed. The shell fixtures move to `codescene_validation_step_data`, which the test module was pushed past the 400-line limit by, and which states each case as the one thing it varies. Cases are added for an uncreated `--artifact-dir` (literal and through a variable) and for a copy and a directory named on one line but in different commands, joined with `;` and with `&&`, beside the control one-liner that does stage the report and must still be accepted.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The report-validation predicates and the tests driving them had both grown past the 400-line limit the Python lint gate enforces, so each was split along a seam rather than trimmed. The shell-text reading is not a fact about this lane. Whether a script is a list of lines, which words a command is handed, and which names capture a command's output are questions about shell text, so they move to `shell_command_scan`. The predicates keep only what is theirs: which directory the validator is handed, whether it was created, and whether the report was copied into it. The four `run`-driven tests move to `codescene_validation_step_test`, where the subject — what the validating step's script must read as — is stated once. The repeated arrange-and-assert shape is extracted so a case still states only what it varies and what it expects. The split is behaviour-preserving: diffing the pre- and post-split implementations over seventeen shapes shows fourteen identical verdicts, and the only three that change are the false passes this round's findings were about, with nothing the old code rejected now accepted. The docs that named the covering module are updated to name both, and the stale cross-reference in the lane-data docstring is corrected. Co-Authored-By: Claude Code <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Address the CodeRabbit findings on the report-delivery contract work.
`command_operands` searched a segment for the command's name, so a
*mention* satisfied it: `echo "cp a b"` names `cp` and runs it not at
all, and a rule reading that mention certifies a copy the script never
made. Anchor the read with `re.match` over a `COMMAND_PREFIX` covering
leading whitespace, `VAR=value` assignments, and the path the executable
was named through. An unrecognised form — `sudo cp`, `xargs cp` — is now
reported rather than accepted, which is the safe direction for a guard.
`names_a_component(prefix=True)` accepted the name in *any* component,
so `"/tmp/${staged}"` and the sibling `"${staged}-old/lcov.info"` both
satisfied the copy-destination check while filing the report where the
validator never looks — contradicting the docstring its caller already
relied on. Require the name to be the operand's leading component.
`assigned_from` credited a name whose value merely spelled the command.
A capture is a command *run*, so require a command substitution.
Also: name the cache-outcome constants the module already exports rather
than repeating their values, and drop the hard-coded `--python` from the
two workflow fixtures, which asserted an interpreter version no test
owns.
Add `shell_command_scan_test.py` — the scan module had no test module of
its own — pinning the mention cases in one direction and the supported
direct forms in the other, so the reading is neither too coarse nor too
fine. The contract suite goes 651 passed, 2 skipped.
Gates: check-fmt, lint, typecheck, markdownlint, nixie,
test-workflow-contracts, test-coverage-artifact, doc-coverage, test — all
exit 0 (nextest 3243 passed, 5 skipped; doctests 82+2+39 passed).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Address the two CodeRabbit findings on the report-delivery work. Ten `CATEGORY_*` constants were `pub` while every sibling label constant in the module is `pub(super)`. Nothing outside `which` names them: the public surface is `RESOLVE_ERROR_CATEGORY_VALUES`, which the application recorder reads. Narrow all ten, so the module states one visibility for its label constants rather than two. The validating-step fixture is built as an f-string, where a lone backslash before a newline is Python's own line continuation and is consumed at parse time along with the newline. The fixture therefore generated a script with the `uv` invocation joined into one line, while the lane writes it continued across two. The cases went on passing — a joined command is still one command — so nothing reported the drift, but they exercised a shape the lane does not have and no longer covered the continued form. Double the backslash so the shell gets its continuation, and pin the property with a test comparing the fixture's script to the lane's own on whether the invocation is continued, rather than on equality, so the two cannot silently diverge again. Proven by liveness probe: with the single backslash restored the new test fails and the other fourteen pass, which is the evidence that the drift was invisible to the existing cases. Gates: check-fmt, lint, typecheck, markdownlint, nixie, test-workflow-contracts (652 passed, 2 skipped), test-coverage-artifact, doc-coverage (98.81%, unmoved), test (3243 passed, 5 skipped) — all exit 0.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… read Four false-accepts in the report-delivery detectors, each reproduced with a constructed lane before a line was changed: - A step that only *prints* the validator satisfied the check that the validator runs. `echo "uv run ... validate_coverage_artifact.py"` contains the script's path and executes nothing, so the step would submit the very report it exists to reject. The script is now read as command segments and the validator has to be an operand of an interpreter — `uv`, `python`, `python3` — rather than a substring of the text. - `cp a b c dir` was read with the *second* operand as the destination. That is another source, so a command whose report never reached the staged directory satisfied the copy check. The destination is the last operand. - A line continuation was read as a line break. The shell removes the backslash and the newline before parsing, so the two lines are one command; read apart, an `echo` continued into a line naming a script passed for the script being run. Continuations are now joined before the split, which is the safe direction — joining can only merge segments, making a rule that asks after one command harder to satisfy. - `name=value` was read as an assignment anywhere in a segment. The shell reads it as one only *before* the command word, so `echo staged="$(mktemp -d)"` assigns nothing and creates nothing, while crediting it recorded a directory the script never made. Only the leading run of assignments is read now. The credential check also required the namespace the gate compares: a token read from `github.` or `vars.` names a same-named value from somewhere the `if` never saw, resolving empty and leaving the action unauthenticated while the step reads as configured. Every fix is driven by a regression case, and each was liveness-checked by re-injecting the defect and watching only the new case fail. The real trunk lane and the clean fixture still report no offenders. Two gate regressions from the above are fixed here as well: the reflowed rows in codescene_validation_step_data.py tripped Ruff ISC004, and the unguarded `HEAD_ASSIGNMENTS.match(segment).end()` tripped ty's unresolved-attribute. Gates: check-fmt, lint, typecheck, markdownlint, nixie, test, doc-coverage all green; test-workflow-contracts 661 passed, 2 skipped (both pre-existing). CodeRabbit findings addressed: the four distinct findings reported on cd4993c.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- netsuke/src/stdlib/which/cache.rs is usually changed with: netsuke/src/stdlib/which/lookup/tests.rs
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
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.
Closes #718.
This branch carries two unrelated pieces of work that the issue joined
because they were observed on the same commit. They have different root causes
and should be read separately.
1. Repair the failing CodeScene coverage check (report delivery)
This is a delivery and ingestion failure, not a coverage failure.
The failing check was
CodeScene Code Coverage (main),timed_out, with output"No valid coverage report found in the build pipeline", reported against the
head of PR #672.
Tracing the lane end to end, the report was generated and uploaded, but nothing
verified it was usable before it left:
[ -f "$file" ]) and then handsit to
cs-coverage upload. Themode: uploadpath performs no content check.was refused there — a different system, hours later — while every step in
the run's logs said
success. The resulting message named neither the stepnor the file at fault.
The repair narrows that failure to the run that produces it. The trunk lane now
stages
lcov.infointo a directory of its own and runsscripts/validate_coverage_artifact.pyover it, between generation and upload.That validator already owns the LCOV contract — it is the hostile-artefact
reader exercised by
make test-coverage-artifact, it executes nothing in thefile it reads, and it requires the directory to hold exactly one
lcov.info.The step ordering is deliberate and load-bearing: it must follow generation,
precede the upload, and precede
Show sccache statistics, whichsccache_contract_test.pyrequires to follow every compile step.The test scope is unchanged: workspace, all features, all targets.
A second, latent defect was fixed in the same lane. The upload carried
installer-checksum: ${{ vars.CODESCENE_CLI_SHA256 }}. This repository declaresno repository variables at all, so that resolved to the empty string and
verified nothing while reading as though it did. Worse, the action's current
revision renames the input to
archive-checksumand rejects a non-emptyinstaller-checksumoutright — so a routine Dependabot bump would have failedthe trunk upload on a value that was already inert.
Workflow-contract coverage added
tests/workflow_contracts/codescene_upload_contract_test.py(backed bycodescene_upload_invariants.py, plus the extractedlane_steps.pyandworkflow_variable_scan.py) holds the lane to the delivery contract: stepordering, the path and format the generator and upload agree on, the credential
being both carried and gated on, checksum inputs staying unset, and no
vars.reference to anything the repository does not declare. The detectors are driven
against synthetic workflow text as well as the repository file, so a detector
that stopped matching cannot pass by finding nothing.
The action-pin assertions follow the convention #731 established: they check the
action's identity and the pin's shape (a full 40-character lowercase
SHA), never the revision — the correct revision is whatever the dependency
updater last pinned, not a value a test can know.
2. Bounded
cwd_modeobservability forWhichResolver(follow-up telemetry)This is a telemetry-contract enhancement, not a resolver correctness defect.
Nothing in the resolver's search behaviour changes.
WhichResolverhas distinct PATH-only, workspace-root, and recursive-workspacesearch domains, but its telemetry recorded only cache outcome, final result, and
error category.
workspace-recursiveandautomisses producedindistinguishable series, so an operator could not tell whether recursive
lookup had been requested or whether it contributed to an outcome.
A closed
cwd_modevocabulary —auto,always,never,workspace_recursive— is now carried on all three series: the
stdlib.which.resolvespan, thenetsuke_stdlib_which_cache_totalcounter, and thenetsuke_stdlib_which_resolution_totalcounter. The label is a telemetryspelling: a manifest writes
workspace-recursive, the label isworkspace_recursive.Cardinality and redaction are enforced, not merely intended. No command
name, filesystem path, workspace name,
PATHvalue,PATHEXTvalue, or otherenvironment value is recorded on any span, event, or metric label. The
application recorder in
src/observability_recorder.rsadmits these countersonly under exact declared label sets, and the recorder tests assert that an
out-of-vocabulary
cwd_mode, acommandlabel, or a missing label isrefused rather than exported.
The cache and resolver metric names are unchanged. The label addition is an
additive-but-deliberate change to the series shape, recorded as an addendum in
ADR-024 so a scraper assuming a fixed label set knows to update.
Search semantics preserved
The four
CwdModecontracts are untouched. The strongest evidence isstructural:
src/stdlib/which/lookup.rsandsrc/stdlib/which/options.rs—which hold the search logic and the
CwdModeenum — are byte-identical toorigin/mainon this branch. The change threads a label through the existingcall sites in
cache.rsand moves the recorders into a newtelemetry.rs.Tests
All four modes are parametrized through recorder-backed and tracing cases —
hits, misses, cache outcomes, span fields, and the failure event. The tracing
case proves the mode is emitted and positively asserts that neither the
command nor the workspace root appears in any captured span field or event.
Where all of this is recorded
docs/adr-024-require-explicit-recursive-workspace-which-search.md—addendum: the telemetry contract, the vocabulary, and the redaction rules.
docs/adr-025-main-owned-coverage-publication.md— the report-validationstep and its ordering contract.
docs/netsuke-design.md— the bounded resolver telemetry.docs/developers-guide.md,AGENTS.md— the gate documentation gap below.One documentation defect found and fixed along the way.
make test-workflow-contractswas described in the developer guide but listedin neither the "Quality gates" section nor
AGENTS.md's pre-commit list.Because
make testruns the Rust suite only andmake lintlints Pythonwithout executing it, a change to a workflow or a workflow-contract suite was
verified by no documented gate at all — the contract could be edited into a
shape it no longer enforced and every listed command would still pass. It is now
listed in both, conditional on the change touching a workflow, a contract suite,
or the coverage validators.
Gate status
Green at
8727b630, the current head:check-fmt,lint(including itsgithub-actions-lintmember),typecheck,test(3243 nextest passed, 5skipped, plus the doctest half: 82 + 2 compile-fail + 39 passed),
markdownlint(142 files, 0 errors),nixie(58 diagrams),test-workflow-contracts(661 passed, 2 skipped),test-coverage-artifact(52 passed), and
doc-coverage(98.81% against an 80% threshold). Each figurecomes from a run whose log carries
COMMIT=8727b630…as its header line.The head was moved to
8727b630by a CodeRabbit cycle that found fourfalse-accepts in the detectors this branch adds — each one a lane that
satisfies the contract's wording while doing nothing it asks. They are worth
naming, because each is a way the contract could have certified a delivery it
had not read:
echo "uv run ... validate_coverage_artifact.py ..."contains the path andexecutes nothing. The script is now read as command segments, and the
validator has to be an operand of an interpreter —
uv,python,python3.cp a b c dirwas read with the second operand as the destination. That isanother source, so a report that never reached the staged directory passed.
The destination is now the last operand.
backslash and the newline before parsing, so the two lines are one command;
read apart, an
echocontinued into a line naming a script passed for thescript being run.
name=valuewas read as an assignment anywhere in a segment. The shell readsit as one only before the command word, so
echo staged="$(mktemp -d)"assigns nothing and creates nothing — while crediting it recorded a directory
the script never made.
Each fix is driven by a regression case, and each was liveness-checked by
re-injecting the defect and confirming that only the new case fails. The real
trunk lane and the clean fixture still report no offenders.
Two notes for a reviewer running gates locally:
make validate-coverage-artifactis unreachable as written: it requires acoverage-artifact/directory that CI never populates, and no target under.github/orscripts/calls it. It fails identically onorigin/main. Thetrunk lane now exercises the same validator through
scripts/validate_coverage_artifact.pydirectly, which is the first livesubject it has had.
CodeScene Code Coverage (main)reportstimed_outon pull requests bydesign.
Coverage (main)triggers on pushes tomainonly; PRs generatecoverage for their local ratchet and neither publish the report nor contact
CodeScene. The check is not required and cannot block a merge. CodeScene check
runs attach to PR heads, not to
maincommits. The trunk lane itself ishealthy: the most recent
mainruns are allsuccess. The validation stepthis branch adds is not yet among them — it exists only on this branch, and
runs on
mainfor the first time after the merge.Hosted verification of the repaired step
Run
35496073097 — a
workflow_dispatchofCoverage (main)against8727b630— is the firsthosted execution of the new validating step. It succeeded, 7m35s for the
job, and the step itself passed in 1s:
The dispatch trigger exists for exactly this purpose: a warm run reads every
cache and writes none, so it exercises the lane without publishing a report or
touching the ratchet baseline. The
ok:line is the validator's only successoutput, and it means the staged directory held exactly one non-symlink
lcov.infoinside its own boundary and that the report passed the LCOVrecord contract. What it was handed was a real report, not a stand-in: the
instrumented run executed 3243 tests across 102 binaries and finished at
92.12% line coverage.
The upload step then ran unskipped and exited 0, so the token is present in
a dispatch context and the report reached
cs-coverage. One caveat a readershould have: the CLI also printed
before reporting
Uploaded code coverage data done.Both lines are present andthe exit code is 0. The message is specific to verifying from a branch: the
most recent
mainrun (35492847811) contains no such line at all — its uploadgoes straight to
Successfully parsed edn data for 257 files.This istherefore an artefact of the dispatch's ref rather than an ingestion problem —
but it does mean the dispatch proves the transport end of the hand-off, not
CodeScene's analysis of this branch, which it does not perform by design.
That distinction matters for what this run can and cannot be cited for. It
establishes that the new step runs and passes in the real lane, on a real
report, in the real image. It does not establish anything new about CodeScene's
side of the boundary — that is the trunk's job, and it is what the failing
check this branch repairs was reporting on.
References
CwdModeobservability toWhichResolver#718Summary by Sourcery
Harden CodeScene coverage publication and expose bounded, redacted search-domain telemetry for WhichResolver without changing lookup semantics.
New Features:
cwd_modetelemetry to WhichResolver spans and cache/resolution metrics while preserving resolver search behaviour.Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: