Summary
export_downgrade_warnings wraps parse-and-diff in a blanket except Exception and
returns a single generic sentence, discarding every per-user downgrade finding it
exists to produce.
Detail
quiltx/acl.py:1349-1364:
try:
replayed = parse_acl_config_text(yaml_text)
diff = compute_diff(replayed, current)
except Exception as exc:
return [
"the generated ACL is not valid input for `quiltx catalog acl` "
f"({format_exception(exc)}); replaying it may change effective access"
]
return [summarize_user_downgrade(item) for item in diff.user_downgrades]
Any failure anywhere in parse or diff replaces the entire result. The two are not
the same kind of problem: a parse failure means the capture is unreplayable, whereas a
resolution failure on one users: key says nothing about the other twenty users whose
access the analysis could still have compared.
Both --yaml's embedded # not captured: notes and the stderr summary read from this
one return value, so the degradation is total in both places.
Impact
Lower than when the review found it. The main trigger was compute_diff raising on a
username/email collision, which PR #107 fixed by making that case resolve with a
warning — captures no longer raise on their own output. What remains is the
generic-line fallback for any other failure, and the two fatal resolution cases
#107 introduced or kept (ValueError for a key that is the email of two accounts, and
for two keys resolving to one account).
Those two are close to unreachable from a capture, since --yaml emits one entry per
user.name. So this is robustness rather than a live bug: the shape is wrong, and the
next thing that raises inside compute_diff will silently cost the export its
per-user detail again.
Proposed fix
Separate the failure modes:
- Catch parse failures around
parse_acl_config_text only, and keep the current
generic message for them — an unparseable capture genuinely has no per-user
analysis to report.
- Let a diff-time resolution failure report itself alongside whatever findings the
rest of the analysis produced, rather than replacing them.
The second part likely wants compute_diff to be able to report an unresolvable user
without aborting, which is a slightly larger change than the try block itself.
Notes
Found by a design-level review of #104/#105/#96/#106 (PR #107); deliberately left out
of that PR's scope because the main trigger had been removed. Independent of the other
follow-ups.
Summary
export_downgrade_warningswraps parse-and-diff in a blanketexcept Exceptionandreturns a single generic sentence, discarding every per-user downgrade finding it
exists to produce.
Detail
quiltx/acl.py:1349-1364:Any failure anywhere in parse or diff replaces the entire result. The two are not
the same kind of problem: a parse failure means the capture is unreplayable, whereas a
resolution failure on one
users:key says nothing about the other twenty users whoseaccess the analysis could still have compared.
Both
--yaml's embedded# not captured:notes and the stderr summary read from thisone return value, so the degradation is total in both places.
Impact
Lower than when the review found it. The main trigger was
compute_diffraising on ausername/email collision, which PR #107 fixed by making that case resolve with a
warning — captures no longer raise on their own output. What remains is the
generic-line fallback for any other failure, and the two fatal resolution cases
#107 introduced or kept (
ValueErrorfor a key that is the email of two accounts, andfor two keys resolving to one account).
Those two are close to unreachable from a capture, since
--yamlemits one entry peruser.name. So this is robustness rather than a live bug: the shape is wrong, and thenext thing that raises inside
compute_diffwill silently cost the export itsper-user detail again.
Proposed fix
Separate the failure modes:
parse_acl_config_textonly, and keep the currentgeneric message for them — an unparseable capture genuinely has no per-user
analysis to report.
rest of the analysis produced, rather than replacing them.
The second part likely wants
compute_diffto be able to report an unresolvable userwithout aborting, which is a slightly larger change than the
tryblock itself.Notes
Found by a design-level review of #104/#105/#96/#106 (PR #107); deliberately left out
of that PR's scope because the main trigger had been removed. Independent of the other
follow-ups.