fix(tools): use top-level tea comment invocation and formalize --login passthrough (#865) #866
Assignees
jason.woltje
Clear assignees
jason.woltje
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mosaicstack/stack#866
Reference in New Issue
Block a user
Delete Branch "fix/865-tea-cli-comment-invocation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #865
Background
tea 0.11.1 has no
commentsubcommand undertea prortea issue; thecorrect invocation is the top-level
tea comment <index> <body>. Calling thenon-existent subcommand form does not error — tea silently falls through to a
no-op and still exits 0, producing a false-success write that a caller
trusting the exit code alone would treat as a durable comment.
tea pr approve/
tea pr rejectlikewise take an optional review comment/reason as a trailingpositional argument, not a
--commentflag, and give no trustworthy signalthat the review state actually landed.
The root defect is a class, not a line: a provider write command's exit code
is never evidence the write is durable. This PR fixes the invocation and puts
every write behind a fail-closed, provider read-back that is bounded, attributed
to this invocation, and paginated.
Investigation note — repo state vs. issue text
The issue's grep of
pr-review.sh(line ~104,tea pr comment ...) wasagainst a stale checkout. On current
main,pr-review.sh's comment path wasalready fixed by #812/#835: it posts through a read-back-verified Gitea REST
comment API instead of any
tea ... commentsubcommand.issue-comment.shstill had the invocation bug exactly as described and is the original fix here.
What the read-back now guarantees
For both issue comments (
issue-comment.sh) and PR approve/reject state(
pr-review.sh), a write counts as durable only after the wrapper independentlyre-reads the created provider record and confirms it was created by this
invocation:
write; the accepted record must have
id > boundary. Gitea ids aremonotonic, so this means "created after this write began" and defeats a
body-only whole-history match that would false-succeed when
teasilentlyno-ops while an identical record already exists (the #865 bug).
id > boundaryalone is only temporal ordering —a concurrent write from a different identity would satisfy it while this
teainvocation created nothing. Both wrappers resolve the acting identityonce via
curl GET /api/v1/user(for the token in use) and additionallyrequire the accepted record's author login to equal that identity. The lone
residual — a concurrent write by the same identity with an identical
body/state inside the boundary window — cannot be closed without a
tea-emitted created-record id (tea 0.11.1 does not reliably provide one) and
is documented in-code; it is strictly narrower than temporal-only matching.
computation and the read-back walk every page (
?limit=&page=1,2,…until ashort/empty page). A record that lands beyond page 1 is still found.
For approve/reject the review must additionally match the requested state
(
APPROVED/REQUEST_CHANGES) and be pinned to the PR's current head commit.There are no
TODO/deferred read-back paths remaining — the earlierapproval/rejection-state deferral has been fully implemented.
Per-file changes
packages/mosaic/framework/tools/git/issue-comment.shtea issue comment …with the correct top-leveltea comment "$ISSUE_NUMBER" "$COMMENT" --repo … --login …form.gitea_resolve_api(resolves both the repo API base and the
/api/v1root),gitea_fetch_all(full pagination),
gitea_authenticated_login(GET /user),gitea_max_comment_id(pre-write boundary), andgitea_verify_comment_posted(requires
id > boundaryAND acting-identity author AND exact body). Failsclosed with a clear stderr message; never trusts tea's exit code. Reads use
curl(urllib hits Cloudflare 403 on this host).--login <name>flag, appended to theteaargv afterthe detected default — tea honors only the last
--login, so an overrideplaced earlier would be silently clobbered.
packages/mosaic/framework/tools/git/pr-review.shapproveandrequest-changes:gitea_resolve_api(+/api/v1root),gitea_fetch_all,gitea_authenticated_login,gitea_max_review_id(pre-write boundary), andgitea_verify_review_submitted(requiresid > boundaryAND acting-identityauthor AND expected state AND
commit_id == PR head). Fails closed; nevertrusts tea's exit code. The prior
# TODO(#865)review-state deferrals areremoved.
API from #812/#835.
--login <name>flag (last-wins ordering) forapprove/request-changes; thecommentaction does not shell out totea.packages/mosaic/framework/tools/git/README.mdtea commentrule, the trailing-positional--commentbehavior ontea pr approve/reject, the--loginlast-winspassthrough, and the new invocation-attribution + full-pagination read-back
guarantees for both comments and review state.
test-issue-comment-readback.sh/test-pr-review-gitea-comment.shGET /user, carryuser.loginon records, andhonor pagination query params. Added regressions that fail against pre-fix
behavior: a concurrent matching write from a different identity must fail
closed (comments and reviews), and a matching review beyond page 1 must
still be found. Existing assertions retained/strengthened.
PowerShell siblings
No
pr-review.ps1orissue-comment.ps1exist inpackages/mosaic/framework/tools/git/— nothing to fix on the PowerShell sidefor these two wrappers.
Verification run
bash -non all changed scripts: clean.shellcheck -x -S warningon all changed scripts: clean.npx prettier --checkon the README: passes.test-*.shsuite inpackages/mosaic/framework/tools/git/: all pass,including the two extended regression harnesses.
Hard firewall compliance
No real account names, tokens, logins, numeric UIDs, or operator hostnames are
present in the diff. Only the provider host
git.mosaicstack.devand genericplaceholders (e.g.
<reviewer-login>,review-bot,primary-reviewer) appear.REVIEW-OF-RECORD: APPROVE
Provenance: This review was performed by an independent reviewer instance, NOT the author of this PR. It was conducted from a fresh, isolated clone (not the author's working checkout), by re-deriving every claim from the diff and from
origin/mainrather than trusting the PR description.Head SHA reviewed:
a27f1fa7df723004498ae5d106b57e5710fcb5ae(confirmed via freshgit clone+git fetch origin pull/866/head+git checkout— exact match).Checklist results
Real fix correct (issue-comment.sh) — PASS. Argv is now built as
TEA_ARGS=(comment "$ISSUE_NUMBER" "$COMMENT" --repo "$REPO_SLUG" --login "$GITEA_LOGIN_NAME")thentea "${TEA_ARGS[@]}"— top-leveltea comment, not the brokentea issue comment ...form. Traced argv construction directly in the diff.Read-back genuine and fails closed — PASS.
gitea_verify_comment_posted()does a realcurlGET against.../issues/$issue_number/comments, checks HTTP 200, then pipes the response to apython3heredoc that filters for a comment whosebodyexactly equals the posted text and requires a positive integerid. Any mismatch raisesValueError→SystemExit(1), which is the function's last statement, so the function returns non-zero. The call site iscomment_id=$(gitea_verify_comment_posted ...) || { echo "Error..."; exit 1; }— it does not trusttea's exit code alone and fails closed on transport error, non-200, or body mismatch.--loginordering claim — PASS, verified true in both scripts. In bothissue-comment.shandpr-review.sh,TEA_ARGSis initialized with the detected default--login "$GITEA_LOGIN_NAME"/--login "$login"first, and the--login "$LOGIN_OVERRIDE"is conditionally appended afterward (TEA_ARGS+=(--login "$LOGIN_OVERRIDE")), consistent with tea honoring only the last--loginon the command line.Scope-reduction claim (pr-review.sh already fixed) — PASS, verified independently against
origin/main(not just the PR's own framing). Onorigin/main,pr-review.sh'scommentaction and the optional comment attached toapprove/request-changesalready route exclusively throughgitea_post_verified_comment(), which does a RESTPOST /issues/{n}/comments, extracts the created commentid, then a REST GET read-back of/issues/comments/{id}and verifies id, body, and repo/PR path match before returning success. Notea pr comment/tea issue commentinvocation exists anywhere inpr-review.shonorigin/mainor in this PR's diff of it. (Note, out of scope for #866 but for the record:tea issue comment/tea pr commentforms do still exist unfixed in sibling scriptsissue-close.sh,issue-reopen.sh, andpr-close.shonorigin/main— those are untouched by this PR and are not part of its claimed scope, but remain open #865-class exposure the reviewer flags for a follow-up issue, not a blocker here.)TODO(#865) deferral judgment — Acceptable, not a hidden regression. The approve/reject state (
tea pr approve/tea pr rejectexit code) is still trusted without a REST read-back of the review object itself; this is explicitly called out in code comments as a TODO(#865) with a concrete suggested follow-up (GET /repos/{repo}/pulls/{pr}/reviews). This is honestly disclosed, not silently left, and is a narrower residual risk than the original bug: the original #865 bug was a fully silent false-success on the comment text (the part users read and act on), which is now fully closed. Review approval/rejection state still ultimately relies ontea's own exit code, which — unlike the no-optea issue comment/tea pr commentforms — is a real, defined subcommand path (tea pr approve/reject) rather than a silently-no-op'd one, so the failure mode is different in kind (a genuine command failing loudly) rather than the same silent no-op class covered by #865's title. Judgment: reasonable to defer, correctly and visibly disclosed, does not resurrect the exact silent-false-success bug for the comment/text channel that #865 was about.No test tampering / no quality-gate bypass — PASS.
git diff origin/main...pr866 -- test-pr-review-gitea-comment.sh test-help-exit-code.shis empty — neither test file is touched by this PR. Ran both directly:test-pr-review-gitea-comment.sh→pr-review.sh durable Gitea comment regression passed(exit 0)test-help-exit-code.sh→help-exit-code regression passed (7/7 wrappers)(exit 0)No
--no-verify, no weakened assertions found in the diff.Self-run verification — PASS.
bash -n issue-comment.sh→ OK (no syntax errors)bash -n pr-review.sh→ OK (no syntax errors)shellcheck -x -S warning issue-comment.sh→ clean, exit 0, zero findingsshellcheck -x -S warning pr-review.sh→ clean, exit 0, zero findings(shellcheck 0.11.0)
Firewall (operator-identity scan) — CLEAN. Grepped the full diff for tokens/secrets/emails/IPs/hostnames and manually inspected every
--login/GITEA_LOGIN_NAMEoccurrence: all are shell variables ($GITEA_LOGIN_NAME,$login,$LOGIN_OVERRIDE) or generic placeholders in prose/usage text (<name>,<reviewer-login>). No literal account name, token, numeric user ID, or operator-tied hostname appears anywhere in the diff.Verdict
APPROVE. The core #865 false-success bug (silent no-op
tea issue comment/tea pr commentmasquerading as success) is genuinely fixed inissue-comment.shwith real fail-closed REST read-back verification. Thepr-review.shscope-reduction claim checks out againstorigin/mainindependently.--loginordering is correctly last-wins in both scripts. The deferred approve/reject state read-back is honestly disclosed as a follow-up TODO(#865) rather than hidden, and is a materially narrower gap than the original bug. No test tampering, both existing regression tests pass, both scripts are syntactically valid and shellcheck-clean, and the diff carries no operator-specific identity.REVIEW-OF-RECORD: APPROVE
Head SHA reviewed:
10fdd49e32I am an independent reviewer, not the PR author. This is a re-review of the remediated head, superseding the earlier approval. A prior independent auditor found a blocking correctness hole after that earlier approval; this review verifies the fix from a fresh, isolated clone rather than trusting the PR narrative.
Check 1 (BLOCKER 1 - issue-comment.sh bounded read-back): PASS.
gitea_resolve_apiandboundary=$(gitea_max_comment_id ...)run BEFOREtea "${TEA_ARGS[@]}"is invoked. Read-back (gitea_verify_comment_posted) requires bothid > boundaryAND exact body match; on no match it returns non-zero with a clear stderr message, and the callerexit 1s. Boundary computation returns 0 when there are no prior comments (max(ids) if ids else 0), and id > 0 still correctly matches any newly created comment.Check 2 (BLOCKER 2 - pr-review.sh review-state read-back): PASS. Both approve and request-changes paths call
gitea_resolve_apiand capturereview_boundary=$(gitea_max_review_id "$PR_NUMBER")before invokingtea "${TEA_ARGS[@]}"(pr approve / pr reject).gitea_verify_review_submittedrequiresid > boundaryANDstate == expected_stateANDcommit_id == <PR's current head sha, fetched fresh via GET .../pulls/$pr_number>. Fails closed (non-zero, stderr message) if no match; caller exits 1. Traced both branches independently in the diff and they are symmetric.Check 3 (login-match omission / TOCTOU judgment): ACCEPTABLE, documented finding (non-blocking). The read-back does NOT verify which account authored the matched review/comment, only id-after-boundary + state + commit_id (+ body for comments). This is a genuine narrow race: a different actor submitting a review with the same state on the same head commit inside the boundary-to-read-back window would satisfy all match criteria and cause a false-positive success report for an action that itself silently failed. The author's stated rationale (tea's local login alias need not equal the actual Gitea account, especially under --login override) is technically sound as a reason not to hard-match login via the local alias. Given this tooling's usage context is a single-writer-per-invocation orchestration script (not an adversarial multi-tenant boundary), the window is a handful of sequential HTTP round-trips, and the README explicitly documents this as a known limitation with a forward-looking mitigation (dedicated per-reviewer credentials), I judge the gap acceptable to ship, but it is a real limitation and not fully closed — a stronger fix would resolve the acting identity from the token itself (e.g. a /user call) rather than omitting identity verification entirely.
Check 4 (no residual exit-zero trust / broken tea forms): PASS.
grep -n "tea issue comment\|tea pr comment"across both scripts matches only a header comment explaining why that form is avoided (issue-comment.sh line 7) - no executable code path invokes the broken subcommand form. Both approve/reject/comment paths always route through the bounded read-back before reporting success.Check 5 (tests genuine): PASS. Ran all three tests from a fresh clone:
Adversarial check: swapped in the origin/main (pre-remediation) issue-comment.sh and reran the new test-issue-comment-readback.sh against it (with the new test file unchanged) - it FAILED (exit 1), because the old script invokes the broken
tea issue commentform which the new test explicitly asserts against, and never performs a boundary-bounded read-back at all. This confirms the new test genuinely discriminates old vs. remediated behavior rather than being tautological. Diffed test-pr-review-gitea-comment.sh against origin/main: every removed assertion line was replaced by a strictly more specific successor (oldApproved Gitea PR #123substring replaced byApproved and verified Gitea PR #123 (review ID 200), matching the new, more informative success message) - no assertion was weakened or dropped without a like-for-like or stricter replacement.Check 6 (lint): PASS.
bash -nclean on all 5 changed/added shell scripts.shellcheck -x -S warningclean (no findings) on issue-comment.sh, pr-review.sh, test-issue-comment-readback.sh, test-pr-review-gitea-comment.sh.Check 7 (firewall): CLEAN. Scanned the diff for credential/token/key patterns, real hostnames, UIDs, and operator identity strings - only match was
"token": "test-only-placeholder"in the two test fixtures (synthetic, not a real credential). No real accounts, logins, numeric UIDs, or operator-tied hostnames found. Only host referenced is the generic provider host git.mosaicstack.dev.Check 8 (no quality-gate bypass): PASS. No
--no-verify, no mocking-around real failures, no commented-out logic to dodge red observed in the diff or the changed test harnesses.Verdict: APPROVE. Both blockers are genuinely remediated with correct boundary-then-write-then-bounded-read-back ordering on both the comment body and review state code paths, tests are real and adversarially verified to discriminate pre/post remediation behavior, lint is clean, and the diff carries no operator-specific content. The login-identity gap in check 3 is a real but narrow limitation, appropriately scoped and documented as a follow-up rather than a blocking defect for this tooling's actual usage pattern.
Replace the tea-based write + boundary/author read-back with a direct Gitea REST POST that returns the created record's id, and verify that exact record. BLOCKER 2 (credential ordering): resolve the acting identity, the write token, and the read-back token from the SAME effective login. A --login override now selects the credential used for the POST, GET /user, and the GET-by-id read-back, so an overridden write is verified against the identity that performed it -- not the host default. Login-name resolution is best-effort and non-fatal (the override always wins; otherwise fall back to the host credential), so exotic/ported hosts still resolve a token. BLOCKER 1+3 (attribution + tautological tests): the write is now POST /issues/{n}/comments or POST /pulls/{n}/reviews (event + body + commit_id == PR head), parsing the provider-returned created id. Verification GETs that exact id and checks author == acting identity and body (comments) or state + commit_id (reviews). Keying on the created id closes the concurrency window: a no-op create yields no id and fails closed with no list-scan fallback, and a concurrent same-identity record has a different id. The review body travels in the review submit, removing the separate detached comment. Tests: the curl stub now models a real server with persistent on-disk review/comment state -- a POST actually creates+persists a record and returns its id, and the read-back reads that same state (no fabricated record for the wrapper to find). Adds same-identity no-op-concurrent and author-mismatch fail-closed cases for both comments and reviews, and >page-1 pagination coverage for both. README "Durable review provenance" refreshed for the REST mechanism. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>MS-LEAD REVIEW-OF-RECORD (process-level author-not-equal-reviewer; posted under shared login for durable provenance)
VERDICT: APPROVE
Reviewed PR #866, branch fix/865-tea-cli-comment-invocation, head SHA
6168f9ac86(round 4, one commit atop prior head9384f0bc0a). Reviewer is an independent web1 subagent, distinct from the coder that authored the change.Scope verified: round-4 delta touches only issue-comment.sh, pr-review.sh, test-issue-comment-readback.sh, and test-pr-review-gitea-comment.sh. detect-platform.sh has an empty diff for this range, consistent with the PR description.
PRIMARY check - silent --login override fallback (the blocker that triggered round 4):
Explicit UNRESOLVABLE --login fails closed at all 4 dispatch sites (issue-comment.sh write site, and pr-review.sh approve, request-changes, comment sites). Verified empirically by running the updated test suite: nonzero exit, no success line, no write POST, and no request under the default identity at every site.
Explicit RESOLVABLE --login override drives GET /user, the write POST, and the exact-id read-back entirely under the override identity, with an explicit assertion in both test files that the default identity never appears in the auth log for that run.
No-override default path still resolves via the host-default best-effort fallback and succeeds as before.
Seam correctness: traced the arg parser. LOGIN_OVERRIDE equal to an empty string (as from --login with an empty value) degrades consistently on both branches - EFFECTIVE_LOGIN falls back to auto-detection and override_explicit (the LOGIN_OVERRIDE:+explicit expansion) evaluates to empty - so there is no scenario where the explicit-fail-closed flag and the effective login source disagree. Confirmed by isolated shell reproduction. Also confirmed by grep that all 4 call sites use the byte-identical LOGIN_OVERRIDE:+explicit expression - no per-site copy-paste divergence.
Regression-test genuineness: checked out the pre-round-4 commit (
9384f0bc), transplanted only the two new test files onto the old scripts, and ran them. Both fail against the pre-round-4 code with the exact silent-fallback symptom the blocker described (comment/review reported success under the host-default identity despite an unresolvable explicit login). This confirms the new tests are a real regression guard, not a tautology.REGRESSION check (round-3 fixes) - all intact:
Gates run and passing:
No blocking findings identified.
NOTE: This APPROVE is one of two required gates. An independent cross-host exact-diff audit at this same head is separately required. Merge remains hard-blocked on the Gate-16 provider-visible author-not-equal-reviewer provenance ruling (shared-login setup) pending owner decision, on the CI queue guard, and on named-executor merge clearance. This RoR is a durable record at head
6168f9aconly; any new head voids it.MS-LEAD REVIEW-OF-RECORD (process-level author-not-equal-reviewer; posted under shared login for durable provenance)
VERDICT: APPROVE
Reviewed PR #866, branch fix/865-tea-cli-comment-invocation, head SHA
2bb3ac4549(round 5, one commit atop prior head6168f9ac). Reviewer is an independent web1 subagent, distinct from the coder that authored the change. This RoR supersedes and voids the prior RoR at6168f9ac(comment 18478), which was overtaken by CI 1958 red at that head.CI: pipeline 1959 at this exact head is terminal-green (ci-postgres, install, sanitization, upgrade-guard, typecheck, lint, format, test all OK).
Round-5 remediation verified. Root cause of the CI 1958 red was PyYAML absent on the node:24-alpine CI image: get_gitea_token_for_login in detect-platform.sh hard-required import yaml, and the round-4 --login path was the first to exercise it in the cold mosaic package test. Fix is an indentation-aware line-parser fallback dispatched on ImportError, with PyYAML kept as the fast path; this also repairs a latent production defect where --login overrides were silently unusable on any host without PyYAML. No assertion weakened or skipped.
Independently verified at this head via a fresh clone:
Gates run and passing at this head: bash -n on all five in-scope scripts; shellcheck -x -S warning clean; prettier on README clean; both test-*.sh suites pass WITH PyYAML and with it forced absent (four runs); cold TURBO_FORCE turbo test filter mosaic 14 of 14 plus 1434 vitest tests.
Two non-blocking follow-ups (neither gates this PR, both tracked separately): (a) the fallback line-parser does not strip an inline hash comment from a scalar value, which fails SAFE (no wrong-login, cross-host, or misattribution direction); (b) test-issue-comment-readback.sh is not wired into the package.json test:framework-shell CI target, a pre-existing gap on main, so its new cross-host and temp-leak assertions were verified manually here but are not yet CI-exercised.
No blocking findings identified.
NOTE: This APPROVE is one of two required gates. An independent cross-host exact-diff audit at this same head
2bb3ac4is separately required and pending. Merge remains hard-blocked on the Gate-16 provider-visible author-not-equal-reviewer provenance ruling (shared-login setup) pending owner decision via Mos, on the CI queue guard, and on named-executor merge clearance. This RoR is a durable record at head2bb3ac4only; any new head voids it.[MS-LEAD web1 independent reviewer — Record of Review]
VERDICT: APPROVE at exact head
4822291707(round-7, branch fix/865-tea-cli-comment-invocation).This is the web1 leg of a dual-gate review (a homelab exact-diff audit runs independently at the same head). Reviewer is a fresh agent distinct from the coder (author != reviewer). Adversarial verification of all nine round-7 items:
1-6 Fallback recognizer (detect-platform.sh, PyYAML-absent path): resolves the same login token as PyYAML or fails closed. Hand-built (not reused-fixture) probes all held: duplicate login name (first-match, identical to PyYAML), nested sub-map shadow, duplicate field, extra-document marker, block scalar, unquoted int/null/bool/float, quoted-digit accepted identically, flow-style list, YAML anchor, and merge-key forms all fail closed or match PyYAML. No fail-open found. Dispatch only on ImportError; PyYAML fast path unchanged; non-str rejected on both paths.
Item 1 TOCTOU (pr-review.sh): after exact review-id read-back (id+author+state+body+commit_id), a genuinely fresh GET of the PR head is required to still equal the submitted SHA; fails closed if the head advanced. head-advanced-race fixture confirmed to simulate a real race.
Item 2 PR-only comment (pr-review.sh comment action): requires pull_request_url kind and rejects a bare issue_url; issue-comment.sh unchanged. comment-plain-issue fixture (issue present, PR absent) fails closed.
Item 3a token out of argv (both wrappers): all authenticated curl use a mode-0600 --config file, unlinked via RETURN traps; no -H Authorization remains; assert_token_not_in_argv confirms the token is absent from spawned argv.
Item 3b strict review-body binding (pr-review.sh): presence + string-type + exact equality, no (body or empty) coalescing; review-body-null fixture confirms.
Integrity: git diff 2bb3ac4..4822291 on all three test files shows zero removed assertions/FAIL/raise lines — additions only; no --no-verify; no skipped/xfail.
Gate suite run independently: bash -n all pass; three test-*.sh suites exit 0; shellcheck -x -S warning zero warnings.
GO is NOT granted by this record. GO stays held under dual-gate discipline until BOTH this web1 review AND the homelab exact-diff audit clear this same head
4822291, CI 1961 is terminal-green, reviewed-SHA==CI-SHA==merge-head, the queue guard is clear, and Mos issues named-executor clearance. GO also remains independently hard-blocked on the Jason Gate-16 provider-visible author!=reviewer provenance ruling. Any push to a new head voids this record.[MS-LEAD web1 reviewer — SUPERSEDING NOTE on comment 18489]
My web1 APPROVE (Record of Review 18489) at head
4822291707is OVERTAKEN. The independent homelab exact-diff audit returned REQUEST_CHANGES at this SAME head, and REQUEST_CHANGES governs regardless of my APPROVE and regardless of CI 1961 being terminal-green.Cause: a residual fallback fail-open in detect-platform.sh. A config containing an unrelated malformed root scalar (for example a bad calendar date 2023-99-99, or a malformed-radix int 0b_ / 0x_, or an invalid-indicator plain scalar) alongside a valid logins block is rejected by PyYAML with a constructor error on the whole document (so PyYAML yields no token), yet the PyYAML-absent line-parser ignores the unrelated key and still emits the valid login token. That violates the whole-document only-ever-more-conservative-than-PyYAML invariant and can emit a credential from malformed config. My web1 review probed structural shadowing and scalar typing but did not probe constructor validity of unrelated keys; the homelab audit caught it. This is exactly why both gates run.
PR #866 is NO-GO at
4822291. Round-8 is in progress to make the recognizer fail closed for the whole document on any non-constructible typed scalar and any invalid plain indicator, with constructor-validity fail-close tests under PyYAML-present and forced-absent. The next remediated head voids CI 1961 and this record. Independent holds also remain: queue state unknown, the Gate-16 provider-visible author-not-equal-reviewer provenance ruling unresolved, no named executor, and the PR body materially stale. No merge.PyYAML raises a ScannerError on a tab used anywhere outside a quoted scalar (leading/trailing/embedded in a plain value, immediately before or after a key colon, or as indentation) and yields no token, accepting tabs ONLY inside single/double-quoted scalars. The conservative block-YAML fallback in get_gitea_token_for_login normalized those tabs away -- via _scalar's top-level .strip(), _KEY_RE's [ \t] separators, _Parser.__init__'s body.rstrip(), parse_seq's body[1:].strip(), and the inline-map emptiness checks -- and still emitted the login token: a credential fail-open in the dangerous direction (less conservative than PyYAML). Extend the whole-document "only ever more conservative than PyYAML, never less" invariant to tab/scanner parity: - _KEY_RE now uses SPACE-only separators (` *:` / `[ ](.*)`), so a tab in a key/value separator makes the line fail to match and the caller fails closed. - Every whitespace-normalization site strips SPACES only (strip(" ")/rstrip(" ")) so a tab survives to a fail-closed guard instead of being silently removed: _scalar top-level strip, quoted-trailing strip, post-comment strip, _Parser.__init__ body rstrip, parse_seq item strip, and the three inline emptiness checks. - _scalar fails closed on any tab remaining in a plain scalar. Tabs strictly inside quoted scalars are preserved verbatim (unchanged parity), matching exactly what PyYAML accepts. Verified empirically against PyYAML 6.0.3: ScannerError for each rejected tab position; string-preserved for quoted inner tabs. Differential fuzz with tab re-included: 0 fail-opens over ~6000 inputs; 0 over-rejection across 220 PyYAML-accepted quoted-tab cases. Adds section 17 to the regression harness (fail-close fixtures for trailing/leading/embedded/after-colon/indentation tabs; parity fixtures for double- and single-quoted inner tabs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Round-9 auditor blockers, both fixed at root cause in the PyYAML-absent line-parser fallback of get_gitea_token_for_login: 1. Control-character fail-open (dangerous): PyYAML 6.0.3's Reader rejects the WHOLE document (ReaderError) if a forbidden C0/DEL control byte {0x00-0x08, 0x0B, 0x0C, 0x0E-0x1F, 0x7F} appears ANYWHERE in the raw stream -- plain scalar, inside quotes, or a comment -- regardless of position, verified empirically against the real installed PyYAML 6.0.3. The fallback previously only guarded tabs and emitted the login token from such documents. Added a whole-document _FORBIDDEN_CONTROL scan on the raw text (before splitlines(), which itself splits on some of the same bytes) so the fallback fails closed identically to PyYAML. 2. Unsigned-exponent float over-rejection: PyYAML's implicit float resolver requires an EXPLICIT sign on the exponent; unsigned-exponent spellings (1.0e10, +1.0e10, -1.0e10, 1.0E10, .5e10, 4.e8) are PyYAML STRINGS, not floats. The fallback's _IMPLICIT_FLOAT pattern allowed an optional sign, misclassifying these as floats and dropping the token. Tightened the regex to match PyYAML's resolver exactly. Also folds in a residual over-rejection found via this round's wide differential fuzz (1767 cases, 0 fail-opens / 0 over-rejections after the fix): a plain scalar starting with "?" not followed by whitespace (e.g. "?x") is a valid PyYAML string, but the fallback's blanket-reject set treated every leading "?" as illegal. Narrowed to match the existing space-aware handling already used for "-" and ":". Adds regression fixtures to test-gitea-login-resolution.sh covering all three fixes under both PyYAML-present and forced-ImportError-absent runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Round-10 auditor blockers in the PyYAML-absent fallback of get_gitea_token_for_login (detect-platform.sh): Blocker 1 (fail-open): the round-9 control-char guard used a hand-rolled C0/DEL subset that missed code points PyYAML's Reader also rejects -- the C1 block (U+0080-0084, U+0086-009F), the surrogate range, and the BMP noncharacters U+FFFE/U+FFFF -- so the fallback still emitted a token from documents PyYAML rejects whole. _FORBIDDEN_CONTROL now uses PyYAML 6.0.3's EXACT Reader.NON_PRINTABLE character class (negated), verified 0-mismatch against real PyYAML across the full BMP + astral range. Blocker 2 (over-reject): the recognizer blanket-rejected any plain scalar containing a flow indicator, but in BLOCK context (where a tea config value always sits) PyYAML treats ',[]{}' as ordinary content and treats '!&*|>#%@`' and quotes as significant only at the first non-space char. The blanket scan dropped tokens PyYAML emits verbatim (internal '!', ',' '[' ']' '{' '}' '#'-not-space-preceded, ':'-not-space-followed). Removed it; the leading-char guard and the ' #' / ': ' / trailing-':' guards keep the fail-open direction shut. Tests: additions-only sections 21 (printable-boundary fail-close for C1 + noncharacters; NEL/U+00A0/astral still resolve) and 22 (internal indicators resolve; leading indicator / ': ' inline map / trailing ':' fail closed). Verified vs real PyYAML 6.0.3 both directions (present as oracle, absent via PYTHONPATH shadow): wide 602 cases 0 fail-open/0 present-mismatch, dense 712 cases 0 fail-open/0 over-reject/0 mismatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Two residual over-rejections in the PyYAML-absent fallback of get_gitea_token_for_login, both fail-closed today but diverging from real PyYAML 6.0.3 for valid inputs. Neither is a fail-open; the fix loosens ONLY to exact PyYAML parity and keeps failing closed elsewhere. Blocker A - embedded printable line break inside a quoted scalar. The fallback split the raw document with str.splitlines(), which breaks at NEL(U+0085), LS(U+2028) and PS(U+2029) -- all PRINTABLE to PyYAML's Reader. Inside a flow (quoted) scalar PyYAML does not break at these: it line-folds a double/single-quoted scalar (NEL/LF/CR -> one space; LS/PS verbatim), resolving one token, while splitlines() cut mid-quote and failed the whole document closed. Replace splitlines() with _split_logical_lines, a quote-aware splitter that reproduces PyYAML's flow folding (scan_flow_scalar_spaces/breaks) and raises on a col-0 doc marker (---/...) in a folded continuation. The same code points UNQUOTED or in a comment still make PyYAML raise, so those stay closed. Blocker B - leading non-specific tag / anchor property. The recognizer blanket-rejected any scalar starting with '!' or '&'. But '! ' (bang + space) and '&name ' are transparent node properties: PyYAML strips them and applies normal implicit typing, so '! x'/'&a x' resolve the string 'x'. Add _strip_properties, consuming <=1 non-specific tag and <=1 anchor (either order) and recursing on the remainder. '!x' (tag handle), '!foo x', non-string nodes ('! 123'), duplicate properties and explicit tags ('!!str x') still fail closed. Fallback now matches real PyYAML 6.0.3 (oracle) on a two-direction differential fuzz of the full Unicode line-break set x {double, single, plain, comment, token-position} and the full leading tag/anchor space: 833 cases, 0 fail-opens, 0 present-mismatches, 52 endorsed fail-closed over-rejects (blank-line-in-quote \n folds; explicit !!str/verbose tags; property+quoted; anchor-without-space). Wrapper scripts pr-review.sh and issue-comment.sh unchanged (0-line diff). Tests: additions-only sections 23 (quoted-scalar breaks) and 24 (tag/anchor properties). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>The PyYAML-absent fallback's _strip_properties consumed a plain anchor ('&name') per-scalar, with no document-scoped registry of declared anchor names. Real PyYAML's Composer rejects a duplicate anchor NAME declared anywhere in the document (ComposerError, whole-document fail), but the fallback would still emit the later token: fail-open (HIGH). Fix: reject every '&'-anchor property unconditionally instead of building a hand-rolled duplicate registry, which guarantees zero fail-open by construction at the cost of a deliberate, endorsed over-reject on a single non-duplicated anchor. The '!' non-specific-tag branch is unchanged ('! x' -> 'x' still resolves).View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.