Repository navigation
Conversation
…n, policy fidelity, and resource hygiene - approve/core: count quorum actors case-insensitively so one identity cannot approve twice through case variants. - execute/base: validate every push refspec destination, not just the last. - approve/bus: accept sync envelopes with seq null. - approve/labels: snapshot the timeline before adding the mirrored label so an intervening maintainer label is not misattributed. - config: validate policy.branch_protection types at load instead of crashing mid-run. - analyze/policy: record unknown (not silence) for secret_scanning on private repos under a paid plan; the free-plan blocked_by_plan finding is unchanged. - collect/github: drop the collaborators field from the GraphQL query and normalise; nothing consumes it. - collect/github: log a warning when the security-extras REST read fails instead of degrading silently. - fleet: clean the preview workdir in a finally block so a failing fixer does not leak full clones. Each fix carries a regression test that fails with the fix reverted.
abu-aska
left a comment
There was a problem hiding this comment.
Review of c73a808 against main at c618f4a: one point on the rebase and three on the changes.
1. Conflicts with main after #16 (mimar to ion). GitHub reports the PR as conflicting. Every touched path moved from mimar/ to ion/, and these literals need to change with it, not just the paths:
safe_push: the new guard checksdest.startswith("mimar/"). Every executor branch on main ision/...(ion/execute/merge.py:33,split.py:106,simple.py:127,autofix.py:21). A rebase that keeps the literal would refuse every executor push.collect/github.py: the new logger isgetLogger("mimar"); main uses"ion". The test asserts onlogger="mimar", so it would still pass while the warning goes to a logger nothing configures. The query hunk also conflicts textually, because main addedlegacyYmlnext toionYml.- Tests:
mimar:approvedandmimar approvebecomeion:approvedandion approve.
I applied the diff to main with those names mapped to ion and resolved the github.py hunk by hand. With GIT_CONFIG_GLOBAL=/dev/null, the six touched test files pass: 107 passed. Only diff.mnemonicPrefix and the other settings in #26 make two unrelated fleet tests fail on main.
2. mirror_label (labels.py). Snapshotting the timeline first narrows the race but does not close it. Suppose a maintainer adds ion:approved after Ion's own label but before the second timeline read. Their event is not in before, and max(created_at) then picks it as Ion's. Two events in the same second also tie. Suggest filtering mine by actor.login equal to Ion's own login (one GET /user per run). That removes the race and the second timeline pagination. #44 (GitHub App identity) would make that login distinct from any maintainer's.
3. Quorum (core.py). Case folding fixes Ada and ada. A bus seat and a GitHub login that belong to the same person still count as two approvers when the names differ. That is filed as #27, together with bus seats skipping the per-repo maintainer check.
4. Config schema (config.py). The comment says "a typo here used to crash". A typo in a key name (required_approval) still validates and is silently ignored, because branch_protection has no additionalProperties: false. GitHub caps required_approving_review_count at 6, so maximum: 6 would catch an out-of-range value at load instead of at the PUT.
Checked with no findings:
- the
safe_pushloop over refspecs int(e.get("seq") or 0)- the
finallycleanup inpreview - the secret-scanning branch (the engine only acts on
drift, sounknownis display-only) - dropping
collaborators(nothing on main reads it)
|
Superseded by #59. The rename made this branch unmergeable; the still-valid fixes were re-applied against ion main there (the mirrored-label fix was dropped — the team's rewrite of mirror_label fixed it better). |
|
Mechanical fixes from an audit read of the approval and write paths, plus the surrounding collect/policy/fleet code. Each one has a test that fails without it.
Changes
mimar/approve/core.py(evaluate): quorum now counts approvers by lowercased actor. A bus seatAdaand a GitHub loginadapreviously counted as two approvers toward quorum.mimar/execute/base.py(safe_push): every refspec destination is checked, not just the last. The broker only sees the last refspec's branch, sogit push origin docs:refs/heads/docs mimar/x:refs/heads/mimar/xwould previously have pusheddocsas well.mimar/approve/bus.py(sync_bus): a bus envelope withseq: nullno longer crashes the sync withint(None).mimar/approve/labels.py(mirror_label): the issue timeline is snapshotted before the mirror label is added. The code previously assumed the latestlabeledevent was mimar's own. A maintainer's label applied in between would be misattributed, and their approval silently dropped from later syncs.mimar/config.py:policy.branch_protectionsettings are validated at load (require_prboolean,required_approvalsinteger >= 0, and the rest). A typo likerequired_approvals: "many"previously crashed mid-run insidelowers_protection.mimar/analyze/policy.py: a private repo on a paid plan now gets a secret-scanning finding ofunknowninstead of no finding at all. The conditional-expression-as-statement only recordedblocked_by_planon the free plan and silently skipped the rest.mimar/collect/github.py: thecollaboratorsfield is dropped from the GraphQL query and fromnormalise. Nothing in the codebase reads it, so it was paying query cost against the API budget on every page for dead data.mimar/collect/github.py(fetch_security_extras): a failed REST read now logs a warning with the repo name before returningNone. Previously a persistent 403 or rate limit degraded tosecret_scanning: "unknown"with no diagnostic anywhere.mimar/fleet.py(preview): the preview workdir is removed in afinallyblock. A fixer that raised mid-loop previously leaked full repo clones under the work directory.Validation
pytest -q: 229 passed (221 existing, 8 new). Each new test was re-run with its fix reverted to confirm it fails.