SEZ9 commented on PR #13002:
URL: https://github.com/apache/seatunnel/pull/13002#issuecomment-6466890296
Thanks @DanielLeens. Both blocking issues are fixed, plus 3 and 4.
**Blocking 1 — validator's first line dropped.** Confirmed against
`VALIDATOR_SYSTEM`, whose contract is `PASS` / `PASS_WITH_NOTES: ...` / `FAIL:
<issue list>` — so the first line carries a real issue and `split("\n", 1)[-1]`
discarded it. Exactly as you said, that removed a genuine defect from the
repair prompt in the one case where two things were wrong at once. The drift is
now prepended and the validator text kept whole, and a test feeds a three-line
`FAIL:` result and asserts every issue survives.
**Blocking 2 — credentials in the repair prompt.** Fixed by routing
`request` and `plan_text` through `redact_credentials`, the same scrubber the
planner path and the session writer use.
One thing I want to be explicit about rather than let the fix imply more
than it does: that scrubber keys on `key = value`, `key: value` and "password
is X". A bare `password Hunter2Secret!` in prose is **not** matched — my first
test used that phrasing and correctly failed. I chose not to widen the regex
here, because it is shared with session persistence and memory, and loosening
it to "password followed by a token" would start redacting things like
"password rotation policy". So the guarantee this PR adds is parity with the
config path, not exhaustive scrubbing. The test docstring records that
boundary, and the tests assert what is actually guaranteed (`password is X` and
`secret_key=X` are removed, the rest of the request survives). Happy to open a
separate issue for hardening the redactor if you think it is worth it.
**Issue 3 — plan names are model output.** Agreed, and this was the sharpest
of the five: exact matching would have condemned a correct config and then
spent three repair rounds moving it *away* from the right connector. Planned
names are now intersected with what `list_connector_names()` registers for that
section. When the registry is unavailable the comparison falls back to names as
given — without a registry there is no way to tell an alias from a real
mismatch, so I kept the pre-existing behaviour rather than silently disabling
the check. Three tests: registered name enforced, unregistered name ignored,
registry unavailable.
**Issue 4 — docstring.** You are right, it described `-CDC` handling the
code never did. I wrote that sentence and it was wrong; removed and replaced
with what the function actually guarantees.
**Issue 5 — test gaps.** Covered by the two above: a multi-line `FAIL:`
merge case, and redaction asserted against the real `_run_fix` prompt rather
than a mock.
The branch has been rebuilt on the updated #13000 and #13001, so it carries
their follow-up commits. Suite from `seatunnel-cli/`: 370 passed, 3 subtests
passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
