test(config): cover utils.* node reconciliation as match (CHE-656) - #133
Closed
congvc-dev wants to merge 15 commits into
Closed
congvc-dev wants to merge 15 commits into
congvc-dev wants to merge 15 commits into
Conversation
![congvc-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Refs CHE-521, CHE-522
Cheese: Linux CI only, no Windows jobs or artifacts (CHE-522). python-quality matrix drops the Windows entry; job has no Windows-specific steps so nothing else changes. Refs CHE-521, CHE-522
ci(workflows): migrate to Blacksmith runners (CHE-522)
playground/backend_manager/app/ is unmodified upstream code that fails the fork's UP ruleset (11 auto-fixable errors: UP017/UP037/UP045), blocking ruff check . repo-wide and skipping every downstream CI step (quality ratchets, pyright, tests) on every PR. Exclude playground/ from tool.ruff rather than auto-fixing, to keep the fork's diff against upstream minimal per the pinned-SHA fork policy (qualification/FORK_MAINTENANCE.md).
* feat(config): per-attempt LLM identity manifest and reconciliation (CHE-491)
Adds the Gate 1 mechanism from the Cheese Work adoption plan: a
credential-free manifest of every model-bearing node resolved for one
attempt, a deterministic canonical-JSON + SHA-256 digest scheme, create-only
storage beside the trace directory, reconciliation of native llm_usage
trace events against the manifest, and a batch accept/reject rule covering
the seven documented reject reasons.
Mechanism only: no qualification pilot was run, and none is claimed to have
passed.
* fix(config): atomic manifest storage, wire attempt manifest into task runner
Addresses CHE-491 review blockers: store_attempt_manifest() previously
checked Path.exists() then wrote via os.replace(), a TOCTOU race letting a
concurrent writer silently overwrite an already-stored attempt manifest.
Storage now uses O_CREAT|O_EXCL so the existence check and the write are one
atomic syscall; a losing writer always raises ManifestAlreadyExistsError
instead of clobbering the winner's bytes.
Wires build_attempt_manifest/store_attempt_manifest into the real inference
route (mcp_server/background/task_runner.py) at launch, worker_start and
termination checkpoints, best-effort and never raising into the live task
path, matching token_meter.record_llm_usage's own contract. Tier inference
falls back to a skip (not a failure) when the resolved LLMConfig doesn't
match a declared TIER_MODELS entry.
Live llm_usage reconciliation (reading back DataEngine-recorded usage mid
or post run) is not wired in this pass -- that requires StorageManager
session-id plumbing beyond this task's scope and is called out as a
follow-up, not silently implied as done.
* feat(config): wire llm_usage reconciliation into the real task runner
Closes the remaining CHE-491 review gap: reconciliation and batch validation
now run against post-run native llm_usage receipts, not just in unit tests.
- artemis/config/attempt_usage_reader.py: read-only lookup of a session's
llm_usage trace payloads straight from the DataEngine SQLite store, via the
same StorageManager(read_only=True) pattern OfflineHistoryReader already
uses. Queries the traces table directly by session_id rather than walking
steps, since llm_usage events are not guaranteed to carry a step_id.
- artemis/config/attempt_reconciliation.reconcile_finished_attempt(): reads
back a stored manifest, reads its session's native usage receipts, and
runs the existing reconcile_attempt/validate_batch as a one-attempt batch.
Returns None (not a verdict) when no manifest was stored, rather than
fabricating a pass/fail.
- mcp_server/background/task_runner.py: Agent(..., session_id=trace_id)
links the DataEngine session that records llm_usage to the same id the
attempt manifest is keyed by. The finally block now also calls
reconcile_finished_attempt and writes attempt_reconciliation_verdict.json
beside the trace, best-effort and never raising into the live task path
(same contract as the existing manifest recording hook), regardless of
whether the task itself succeeded, failed, or was cancelled.
Original llm_usage receipts are never mutated or summarized away; the
verdict file is additive evidence alongside them.
* fix(config): close credential leak, node-alias, and identity-bypass gaps in Gate 1 manifest
Fixes 4 blocking findings from independent review of the attempt manifest
and reconciliation mechanism:
- env_overrides could copy credential-shaped ARTEMIS_* vars (e.g.
ARTEMIS_TENANT_TOKEN) by raw value into the hashed, on-disk manifest.
Now reported as {"set": bool, "last4": ...} like provider credentials.
- validator_pixel_safety_net's LLM call is traced under the enclosing
@trace scope name (safety_net_pixel_validation), so it reconciled as
unmapped_call despite being a real, correctly-identified call. Added
an explicit node alias map.
- validate_batch's missing_identity check accepted the "unknown, not a
git checkout" source_sha sentinel and enabled-but-untiered custom
models as if they were valid identities. Both now reject.
- Documented (no code change) that reconcile_finished_attempt's
single-attempt batch can never exercise mixed_tier; that path stays
covered by validate_batch's own unit tests.
* fix(artemis): reconcile soft-defaulted nodes and add batch-by-run_id path (CHE-491)
_manifest_expected_source now derives the expected provider:model from a
disabled node's would_resolve_to (e.g. validator_pixel_safety_net soft
defaulting to lightweight_judge_default()) instead of always returning None
for disabled entries, so a legitimate soft-defaulted receipt reconciles as
match instead of falsely rejecting the batch.
Adds reconcile_attempt_batch_by_run_id: a real, tested, general-purpose
multi-attempt reconciler over attempts sharing a run_id, scanning
TRACES_DIR for candidate manifests. This is the production-reachable home
for the mixed_tier rejection path, which reconcile_finished_attempt can
never exercise (always a single-attempt batch). Not wired into
task_runner.py since no real multi-attempt call site exists yet.
* fix(artemis): enforce tier pinning on soft-default reconciliation, make run_id grouping reachable (CHE-491)
Sol's fresh review of 619d669 found two P1 gaps: a soft-defaulted node's
would_resolve_to was trusted as "expected" without checking its tier against
the manifest's own declared tier, letting a sol-pinned attempt silently
accept a Luna-tier receipt via validator_pixel_safety_net's soft default.
Separately, reconcile_attempt_batch_by_run_id had no real caller that could
ever produce two manifests sharing one run_id, since run_task/_record_attempt_manifest
hardcoded run_id=trace_id.
_manifest_expected_source now cross-checks would_resolve_to's tier against
the manifest's own tier and returns None (routing to mismatch) on disagreement
or an unmapped tier. run_task and _record_attempt_manifest gain an optional
run_id parameter (default preserves today's 1:1 behavior) so a real worker
call can opt into shared-run grouping without any new orchestration logic.
* fix(artemis): propagate run_id through real worker CLI and reconcile batches at termination
CHE-491 Gate 1: the CLI (`python -m mcp_server.background.task_runner`) had
no --run-id flag and mobile_run_task never passed one to the spawned
subprocess, so real multi-attempt worker output never reached
validate_batch. Termination only ran single-attempt reconciliation.
- mcp_server/background/task_runner.py: add --run-id to the CLI parser
(factored into _build_arg_parser), thread run_id through to run_task;
_reconcile_and_store_verdict now also runs
reconcile_attempt_batch_by_run_id when run_id differs from trace_id,
storing a sibling attempt_reconciliation_batch_verdict.json, isolated
from the single-attempt guard.
- mcp_server/tools/task_runner.py: mobile_run_task accepts an optional
run_id and forwards it to the subprocess as --run-id.
- tests: real-CLI-parser and real-termination-path regression tests
proving a mixed-tier batch sharing a run_id is rejected, and that the
batch path is a no-op for unmodified single-attempt callers.
* feat(artemis): extend Gate 1 evidence hooks to daemon-dispatched runs (CHE-491)
Extract shared record/reconcile hooks into attempt_lifecycle_hooks.py so the
standalone and daemon-dispatched execution paths use one contract instead of
duplicating it. Thread run_id end-to-end through the daemon dispatch chain
(daemon_client -> RunRequest -> task_queue_service -> worker CLI) and wire
the hooks into execute_task(), the single point both paths converge on.
* fix(artemis): default identity + reconcile-before-cleanup on CLI run path (CHE-491)
Terra review on 2a920da flagged two P1 gaps in execute_task():
default --standalone (no --session-id) silently recorded zero Gate 1
evidence, and reconcile_and_store_verdict ran after agent.clean() so a
clean() failure could suppress the reconciliation verdict. Generate a
canonical trace identity when none is given, and reorder the finally
block to reconcile first with cleanup independently guarded, matching
mcp_server/background/task_runner.py's existing pattern.
* fix(artemis): stop leaking generated session id across CLI run.py calls (CHE-491)
A default-route execute_task() call (no --session-id) generated a
fallback UUID and unconditionally wrote it to the process-global
ARTEMIS_SESSION_ID env var. A second default-route call in the same
process then inherited the first call's leaked identity via the
os.getenv("ARTEMIS_SESSION_ID") fallback, merging two attempts under
one trace id (manifest silently skipped as a duplicate create-only
key; reconciliation read merged receipts for both).
Only persist to the env var when the identity came from an explicit
session_id arg or a pre-existing env var; a freshly-generated fallback
stays local to the invocation. Adds a regression test proving two
consecutive default calls get distinct trace ids and both manifests
get stored.
* fix(artemis): restore prior ARTEMIS_SESSION_ID instead of conditional write (CHE-491)
The previous fix only skipped the process-global env write when
effective_sid came from a generated fallback. An explicit session_id
argument was still written with no restoration, so a later default-route
execute_task() call in the same process inherited it via
os.getenv("ARTEMIS_SESSION_ID"), colliding two distinct attempts under
one trace id. Snapshot the env var before the call and restore it (or
clear it) in an outer finally covering the whole function body, so no
identity source -- explicit, env-inherited, or generated -- can leak
into a later invocation.
---------
Co-authored-by: congvc-bot[bot] <3634010+congvc-bot[bot]@users.noreply.github.com>
fix(artemis): add anthropic tier to Gate 1 TIER_MODELS (CHE-639)
…85506fd4 fix(artemis): drop temperature for Anthropic models that reject it (CHE-640)
…ce contract (CHE-537) (#3) * feat(qualification): pin qualification inputs and fork maintenance contract (CHE-537) Publishes the versioned manifest for the pocket-actual save/relaunch qualification journey (fork SHA, model tiers, entry point, fixture, and evidence schema references), its NL task/assertion journey spec including a deliberately false negative control, and the fork's pinned-SHA maintenance contract (weekly upstream review + manual pin promotion). Reuses the Gate 1 manifest/reconciliation mechanism and tier vocabulary from PR #1 (CHE-491) by reference/digest; does not port or rebuild it. No host install, provider calls, device operations, or qualification execution — authoring only, per CHE-537's approved scope. * fix(qualification): swap dead-on-arrival Sol tier for Luna, per CHE-388 plan CHE-388's plan says "compare Luna and Terra model tiers" but the manifest pinned Terra+Sol with no documented rationale. Sol resolves to an OpenAI-backed model, and the workspace announcement bars all OpenAI routing indefinitely -- pinning it as an active qualification target would freeze a batch that can never execute. Swap to Luna+Terra to match the plan and record the Sol exclusion (reason + unblock condition) instead of silently dropping it. Also refreshes review_contract to reflect the actual re-staffed reviewer (mbp-claude-sonnet-5) and the current CHE-540/541/542 gate state, both of which had drifted same-day per issue comments 01a0a83b/01a0a83f. Addresses blocking findings 1 (stale-base rebase, handled separately) and 2 from mbp-claude-sonnet-5's review (comment 01a0a844). --------- Co-authored-by: congvc-bot[bot] <3634010+congvc-bot[bot]@users.noreply.github.com>
…-540) (#12) Manifest revision v2 per CHE-540 dispatch: fills candidate_app.app_sha, apk_digest_sha256, package_name and dependency_lock.digest_sha256, held null by CHE-537 pending the execution stage. Candidate is cheese-work/pocket-actual @ 2c42481778e6f9b88a9502f55decdbe4e1a76e83 (main HEAD), built via ./gradlew :composeApp:assembleDebug on X99, no device operations performed. Updates the CHE-537-era test asserting these fields as a null hold to assert the pinned values instead. Co-authored-by: congvc-bot[bot] <3634010+congvc-bot[bot]@users.noreply.github.com>
…d context_management (CHE-643) (#11) ChatAnthropic._make_message_chunk_from_anthropic_event reads event.context_management off message_delta stream events and calls .model_dump() on it unconditionally. That name is only declared on ChatAnthropic itself (a plain request-side dict); Anthropic's RawMessageDeltaEvent has no typed field for it, so when the API includes that block in a response it arrives as a raw dict via Pydantic's extra="allow" and .model_dump() raises AttributeError, discarding the whole in-flight response. Confirmed present on langchain-anthropic's latest release, so a version bump would not fix it. Patches _make_message_chunk_from_anthropic_event at Anthropic model construction time to normalize any raw-dict context_management/container into something model_dump()-safe before delegating to the original method. Co-authored-by: congvc-bot[bot] <3634010+congvc-bot[bot]@users.noreply.github.com>
…stead of silently using the operator model (CHE-646) (#13) * fix(object_detector): resolve detector LLM from utils config instead of silently using the operator model (CHE-646) object_detector.py called get_llm(ctx, name="object_detector") without is_utils=True, so it always raised AttributeError (object_detector is a field of LLMConfigUtils, not LLMConfig) and a bare except Exception silently fell back to the operator model on every call, even when a detector was properly configured. Pass is_utils=True to resolve the correct field, and narrow the except clause to (ValueError, AttributeError) with a logged warning so an unconfigured detector still falls back but is no longer silent. precondition_pixel.py had a structurally identical but functionally dead fallback in _init_llm_and_prompt: "validator" is not a real LLMConfig field, so the fallback branch could only ever raise itself. Removed the unreachable branch and documented why validator_pixel_safety_net's own get_agent default makes it unnecessary. Added unit tests covering: configured detector resolves via utils (not operator), unconfigured detector logs a warning and falls back to operator, and the pre-fix is_utils=False path still raises AttributeError (regression guard). * fix(image_processor): stop hiding the operator model behind an impossible config lookup (CHE-646) The audit for CHE-646 found the same shape at ImageProcessor.run: get_llm(name="image_processor") wrapped in a bare except that resolved the operator model. There is no image_processor node on LLMConfig or LLMConfigUtils, so the lookup raised AttributeError on every call and the agent has only ever run on the operator model. Behaviour is unchanged: call the operator node directly and document why. Giving the image processor its own configurable node is a config-schema change and is tracked separately. --------- Co-authored-by: congvc-bot[bot] <3634010+congvc-bot[bot]@users.noreply.github.com>
Author
|
Opened against the wrong repo by mistake (automation default picked the upstream parent instead of the fork). Closing; correct PR opened at cheese-work/artemis. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CHE-656 asked to decide + close a claimed Gate 1 manifest coverage gap: that
build_attempt_manifestnever tracks the fourLLMConfig.utilsnodes (outputter/hopper/video_analyzer/object_detector), causing every real run to reconcile asunmapped_call.Finding: the gap does not exist in current code.
build_attempt_manifesthas tracked all four utils nodes with the same tier-matching treatment as the twelve required agent nodes since the mechanism's original commit (5eae089, CHE-491) — they live under a separatemanifest["utils"]key (parallel tomanifest["nodes"]), andreconcile_attemptalready merges both dicts ({**manifest.get("nodes", {}), **manifest.get("utils", {})}) before matching usage events by node name — no separate handling, no gap.The
unmapped_callevidence quoted in the issue only showedmanifest["nodes"](the agent-node dict), which by design never contains utils entries — that's not the same as the manifest lacking them entirely.Decision (CHE-656 AC1/AC2)
utils.*nodes are included in manifest node coverage, with the same tier/source verification as agent nodes. No production code change needed —attempt_manifest.pyandattempt_reconciliation.pyalready implement this correctly.Change
Regression coverage only (CHE-656 AC3), closing the gap that no test actually asserted this:
test_attempt_manifest.py: enabledoutputter/hopperutils nodes get the sameenabled/resolved_tier/configtreatment as an agent node.test_attempt_reconciliation.py: a real usage event fornode="outputter"/"hopper"reconciles asmatch, notunmapped_call.AC4 (real pilot run)
Out of scope for this PR — running an actual qualification pilot against a live device/model is separate execution work (see
qualification/FORK_MAINTENANCE.md), not a code-level fix. Per the issue's own caveat, this was the last known blocker; recommend re-running the CHE-491 qualification pilot now, as its own follow-up.Test plan
uv run pytest tests/unit/config/— 73 passed