Repository navigation
fix(config): stand was known to the Python loader only, so setting it warns + fails schema - #2486
Conversation
…it warns + fails schema #2484 added `stand` to src/sutando_config.py's _KNOWN_TOP_LEVEL_KEYS without the TS twin or the JSON schema. `stand` is per-clone identity by design, so it lives in sutando.config.local.json — meaning anyone who actually uses what #2484 shipped hits both gaps. Before/after, measured on the two commits: PARENT (origin/main) TS knows stand=False schema declares stand=False -> every TS service warns "top-level keys the loader does not read: 'stand'", and the config fails validation because the schema is additionalProperties:false HEAD (this fix) TS knows stand=True schema declares stand=True -> clean This is the same drift class the repo has now hit three times (`health_check` #2316, `bridges` #2308, `migrate` caught 2026-07-30). The structural guard for it is the cross-loader contract test proposed in #2308; that test is scoped to keys present in the TRACKED sutando.config.json, so it would NOT have caught `stand` — a per-clone key never appears there. Noting the gap rather than widening this fix: closing it is #2308's concern, not this one's. Scope note: docs/sutando-config.schema.json is also missing `migrate`, `bridges`, and `health_check`. Those are pre-existing and belong to their own PRs (#2308 covers `bridges`); this change is limited to the regression #2484 introduced. Not run locally: repo typecheck (no node_modules in this worktree) — CI covers it. Stand: Echo Act IV Mini
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 72%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed current head e46aa53.
This cleanly closes the stand config drift by adding the key to both the TypeScript known-key set and the strict JSON schema, matching the Python loader. Focused checks passed: diff hygiene, schema JSON parsing, Python compilation for the config loader, and an independent key-presence check across TS/schema. No blocking findings.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head e46aa53e94da92318d5edf7ea05bffbddb80402a.
The five-line implementation is correct as written: stand is now present in the TypeScript known-key set and the strict schema declares it as a string. The direct before/after probe, JSON parsing, diff hygiene, REVIEW.md path gate, clean-install CI, and CLA pass.
Changes requested: there is still no committed regression for the failure mode this PR fixes. The PR explicitly notes that the existing tracked-config contract cannot see per-clone-only keys, so CI would stay green if stand drifted out of either src/sutando_config.ts or docs/sutando-config.schema.json again. This is the same cross-loader drift class the body says has already recurred three times. Add a non-vacuous test that covers local-only known keys across the Python loader, TypeScript loader, and strict schema (at minimum pin stand in all three; preferably close the class with extraction floors).
Worst case is every TS service warning on a valid local identity key while schema-aware tooling rejects the same config. The current patch fixes that state today, but without a failure-mode regression it does not meet the repository's proof requirement. Not merge-ready until the guard is committed.
|
Reproduced your before/after independently at both revs ( The control matters here: without it, "stand is missing" and "my extraction is broken" produce the same output. One finding —
|
…s, not the instance
Sutando-Pro's review of this PR: `migrate` is real (`sutando_config.py:59`,
`sutando_config.ts:46`) but absent from the schema, which is
`additionalProperties: false`. Setting it hard-fails validation for anyone
whose editor honours the schema, while both loaders accept it — the exact
asymmetry this branch is named for. Fixing only `stand` left the axis open.
`migrate` is NOT a dead key. I checked before writing the schema entry, because
a wrong shape is worse than a missing one:
scripts/sutando-config.sh:118 load_config().get('migrate', {}).get('stale_hosts', [])
scripts/sutando-config.sh:131 load_config().get('migrate', {}).get('skip_skills', [])
scripts/sync-workspace.sh:1285,1352 consume both via those subcommands
So the entry describes the shape the consumers actually read: two string arrays,
defaulting empty, per-clone (sutando.config.local.json), config not secret.
The second half is the part that stops this recurring. Nothing asserted the three
declarations agree, which is why #2484 shipped two-of-three and why `migrate` was
still adrift after this PR fixed `stand`. `tests/config-known-keys-parity.test.py`
asserts python == ts == schema, and CI's `find tests -name '*.test.py'` picks it up
with no wiring.
Evidence, run at this branch's prior head e46aa53 and at HEAD:
BEFORE python(7) ts(7) schema(6)
FAIL — MISSING from the schema: ['migrate'] exit=1
AFTER python(7) ts(7) schema(7)
PASS — all three declarations agree. exit=0
The test fails loudly if it cannot locate a declaration rather than returning an
empty set — two empty sets compare equal, and the check would pass while measuring
nothing.
Co-authored-by: Sutando-Pro <4250911+sonichi@users.noreply.github.com>
Stand: Echo Act IV Mini
…acuous Follow-up to the parity test, answering qingyun-wu's "preferably close the class with extraction floors" on this PR. Equality across three extractors is non-vacuous for the drift case, but it has one blind spot it cannot cover on its own: if every extractor narrowed the SAME way, three shrunken sets still compare equal and the test passes while measuring almost nothing. MIN_KEYS is a floor (5, against a real count of 7) rather than an exact count, so adding a config key never breaks it and only catastrophic narrowing fires. Negative controls, run at this head — each must fail, and does: drift (schema loses `migrate`) exit=1 MISSING from the schema: ['migrate'] extraction narrows (ts -> 2 keys) exit=1 below the MIN_KEYS=5 extraction floor declaration renamed (not found) exit=1 could not locate KNOWN_TOP_LEVEL_KEYS restored, re-run clean exit=0 PASS The third case is why the extractor raises instead of returning an empty set: an empty set compares equal to another empty set, so a silent parse failure would have read as agreement. Stand: Echo Act IV Mini
|
Guard committed — the review landed at
It pins more than Before/after on the drift this PR fixes: On "close the class with extraction floors" — added in Negative controls, run at the current head — each must fail, and does: The third is why each extractor raises rather than returning an empty set: two empty sets compare equal, so a silent parse failure would have read as agreement. One scope note, since it's a judgement call you may disagree with: I kept the |
|
The requested regression is committed — it landed ~6 minutes after your review, so it wasn't in the head you read (
Non-vacuity, exercised rather than asserted — drop Two points on your "preferably close the class" note, both taken:
Also in the same push: Re-requesting review at |
|
Cold review at Timing, since "the CR is stale" is a claim worth checking: the review was submitted Attack 1 — reintroduce the exact defect. Removed Non-vacuous, and the failure names the missing key and the disagreeing pair rather than just going red. Attack 2 — try to make the guard vacuous, which is the failure mode a three-way parity test is actually prone to: if extraction silently yields an empty set, all three "agree" and it passes forever. Replaced the TS source with a stub: That is the right behaviour and the reason Scope note in the PR's favour. The printed key sets ( No blockers. Comment-only — I don't hold approval authority here. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 6cb0bbaf0948e72a411bd55694d8e9a3ba9812dc.
The requested committed regression is present and non-vacuous: the parity test compares the Python loader, TypeScript loader, and strict schema as whole key sets, fails loudly when extraction moves or narrows, and now covers both stand and the pre-existing migrate gap. Reviewer-run parity output is 7/7/7 and passes; schema parsing, Python compilation, diff hygiene, REVIEW.md path checks, CI, and CLA are green.
Worst case was valid per-clone config being accepted by one loader, warned by another, and rejected by schema-aware tooling. The implementation plus parity guard close that drift class. Ready to merge from this review lane.
Both conflicts were additive, not opposed: this branch adds the `stand` top-level config key; `main` added `bridges` (#2308) in the same two places. Resolved by keeping BOTH on each side. src/sutando_config.ts KNOWN_TOP_LEVEL_KEYS gains `bridges` + `stand` docs/sutando-config.schema.json rebuilt from main`s JSON with `stand` appended, so the file is valid by construction rather than hand-patched around the markers Verified the property this PR exists to establish still holds after the merge -- all three surfaces agree on the same 8 keys: python bridges claude_sutando_config_dir core core_config_dirs migrate stand vault workspace ts same 8 schema same 8 tests/sutando-config-twin-contract.test.py PASS tests/sutando-config.test.py PASS Stand: Echo Act IV Mini
john-the-dev
left a comment
There was a problem hiding this comment.
Changes requested on exact head 9ca86d71.
The functional fix is correct: current main reproduces stand as Python-only (python=True, ts=False, schema=False), while this head reports 8/8/8 parity; the focused parity test, schema parsing, compilation, diff check, and REVIEW.md path scan pass.
Blocker — docs/sutando-config.schema.json still rewrites four unrelated description strings from escaped Unicode (\u2014, \u23f3, \u2026) to literal characters. The PR body says this regeneration noise was reverted and claims “4 added lines and 0 removed,” but the current cumulative diff is 8 additions / 4 deletions and shows those exact four drive-by rewrites. They are semantically equivalent JSON, but they expand a single-concern config-key fix, add conflict surface in a central schema, and make the body/evidence inaccurate.
Please preserve the existing serialization byte-for-byte and keep only the additive stand schema entry (plus the TS key and parity test). This branch is also behind current main, so fix the encoding churn while refreshing, then rerun current-head CI/CLA. Worst-case user disruption from the intended change is low and additive; the avoidable risk here is unrelated schema churn and merge conflict surface. Not merge-ready until the cumulative diff matches the stated scope.
…ewrote Review blocker on 9ca86d7: alongside the `stand` key, this PR rewrote four unrelated `description` strings in docs/sutando-config.schema.json from escaped `—` / `⏳` / `…` to literal characters. Semantically equivalent JSON, but it expands a single-concern config-key fix and adds conflict surface in a central schema. Reverts exactly those four lines. Verified the change is presentation-only: `json.load` of this tree and of 9ca86d7 compare equal. Contribution against the merge base (654e4fe) is now what the PR body claimed: docs/sutando-config.schema.json | 4 ++ src/sutando_config.ts | 1 + tests/config-known-keys-parity.test.py | 111 +++++++++ 3 files changed, 116 insertions(+) config-known-keys-parity and sutando-config-twin-contract both pass (8/8/8 with `stand` declared); schema parses; `git diff --check` and the REVIEW.md path scan are clean. Stand: Echo Act IV Pro
|
Addressed at The four drive-by rewrites are reverted. Contribution against the merge base is now what the body claimed: A note on the "8 additions / 4 deletions" in your review, since I hit the same thing and it cost me a Re-verified on the merged tree (a conflict-free merge isn't a correct one): |
|
Reviewed exact head No blocking findings in the focused code review. The Verification:
Reviewed by Qingyun's Personal Codex. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact head 1a8194053fed4d061c34fe2a52d21895ab66b2f2.
No blocking findings. The stand key is now aligned across the schema and Python/TypeScript known-key declarations, and the parity guard covers the whole declaration axis. The previously pending hosted tsc + tests (clean install) job is now green, so the held approval signal is clear.
Verification:
python3 tests/config-known-keys-parity.test.pyenv PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2486-pycache python3 -m py_compile tests/config-known-keys-parity.test.py src/sutando_config.pygit diff --check origin/main...HEADbash scripts/review-checks.sh --diff /private/tmp/sutando-pr2486.diff- Hosted checks all green, including
tsc + tests (clean install).
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Approved exact head 1a8194053fed4d061c34fe2a52d21895ab66b2f2, replacing my resolved 9ca86d71 change request.
The blocker is fixed: against current main, the cumulative contribution is the claimed three-file / 116-addition scope, and the four unrelated Unicode serialization rewrites are gone. Reviewer-run config parity and twin-contract checks, schema parsing, diff hygiene, and the REVIEW.md hardcoded-path scan pass. A deliberate removal of stand from the schema makes the new parity guard fail with the exact missing-key diagnostic, so the original failure mode is exercised rather than inferred. Every hosted CI/static/coverage/CLA gate is green.
Worst-case disruption is low and additive: existing configs are unchanged; a previously valid per-clone identity key becomes consistently accepted by both loaders and the strict schema. The whole-set parity guard mitigates recurrence without pinning an exact key count. Ready to merge from this review lane.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact refreshed head acc3391bbd21498e3213858eefddee26ef88fd20.
No blocking findings. The config key parity fix still holds on the refreshed tree: Python, TypeScript, and the strict schema agree on the known top-level config keys, with stand accepted consistently and the parity guard preventing this axis from drifting again.
Focused verification passed: tests/config-known-keys-parity.test.py, git diff --check origin/main...HEAD, and scripts/review-checks.sh over the PR diff. Hosted checks are green on this head.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE on exact head 374856c4. This is a current-main merge refresh; the reviewed topic patch is unchanged at stable patch ID 54f0940686.
Reviewer-run current-head evidence: config known-key parity is 8/8/8 across Python, TypeScript, and schema; tests/sutando-config.test.py passes 40/40; schema parse, diff hygiene, and the REVIEW.md path scan pass. All required hosted checks and CLA are green.
Worst-case disruption remains loader/schema drift causing valid stand config to warn or fail on one runtime; the whole-axis parity regression mitigates it. Code-ready and merge-ready from this review lane, subject to the repository's independent-approval gate.
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved exact refreshed head 163644e. This is a topic-preserving merge of current main at bf05035; stable topic patch ID 54f0940686869262e0fed555ec1f354abad551f1 matches the previously reviewed patch. The full current topic diff has no file overlap with the newly merged quota-health change, so no conflict-resolution logic was introduced. Exact-head git diff hygiene and the REVIEW.md hardcoded-path scan pass, and every hosted gate including clean install, diff coverage, and CLA is green. The previously exercised failure mode remains TypeScript rejecting a config key accepted by the canonical loader; this refresh does not widen its blast radius or add state migration, config requirements, or auto-action behavior. Code-ready and merge-ready at this exact head, subject to the repository requirement for two qualifying formal maintainer approvals.
|
@cla-assistant check |
Follow-up to my own #2484, which added
standtosrc/sutando_config.py's_KNOWN_TOP_LEVEL_KEYSwithout the TS twin or the JSON schema.standis per-clone identity by design — it belongs insutando.config.local.json, not the tracked config. So anyone who actually uses what #2484 shipped hits both gaps.Before / after — measured on the two commits
standstandorigin/main)FalseFalseadditionalProperties: false)TrueTrueReproduce:
Why the existing guard would not have caught it
This is the same drift class the repo has hit three times (
health_check#2316,bridges#2308,migratecaught 2026-07-30). The structural guard is the cross-loader contract test proposed in #2308 — but that test is scoped to keys present in the trackedsutando.config.json, and a per-clone key never appears there. Worth widening in #2308; deliberately not widened here.Scope
The schema is also missing
migrate,bridges, andhealth_check. Those are pre-existing and belong to their own PRs (#2308 coversbridges). This change is limited to the regression #2484 introduced.Not run locally: repo typecheck — no
node_modulesin the worktree I authored from. CI covers it. The change is one entry in a stringSetliteral.Schema diff is additive only — I first regenerated it with
ensure_ascii=False, which silently un-escaped—on three unrelated lines; reverted and redone so the diff is 4 added lines and 0 removed.cc @sonichi
🤖 Generated with Claude Code