Repository navigation
Fix repair plans from the HTML report for three or more nodes - #168
mason-sharp wants to merge 1 commit into
Conversation
A plan rule matches rows by primary key and diff_type in every node pair; it cannot name a pair. The page script did not allow for that: - In a truncated report, a rule for a shown row also repaired a row with the same key and type that the report hid in another pair, and a range built from keys of different pairs could cover hidden keys. - It kept one entry per key, from the first pair that had it, and wrote it as a row_override, which has no diff_type filter. A key in several pairs got a rule only for the first, and the override also matched the key's rows in the other pairs, where table-repair could reject the plan. - It read a row's chosen action from the first control on the page with that key, whatever its pair. The report now marks each shown row whose key and type the report hides in another pair (hidden_twin), counting rows with no visible difference, which the page never shows but table-repair still sees as row_mismatch. The script makes one entry per key and type, reads each row's action from its own pair's section, and uses only rules when the report has more than one pair. It leaves out a row when a rule for it would change a row nobody chose: its twin is hidden, it is selected in only some pairs, or it has a different action in another pair. The YAML lists those rows in a comment, and default_action is skip. Older bugs in the same code are fixed as well: - apply_from, coalesce_priority and the pick_freshest tie got node names, but table-repair only understands n1 and n2, the first and second node of each pair. With other node names table-repair rejected the plan, and for a pair such as n2/n3 a node name could be read as the wrong node of the pair. - A plan built from some selected rows kept default_action keep_n1, so the rows not selected were repaired too, and one missing on n1 made table-repair reject the plan. It now has default_action skip. - YAML reads U+0085, U+2028 and U+2029 as line breaks, and neither JSON.stringify nor Go's json.Marshal escapes all of them. In a quoted key the break became a space and named another row; in a comment the rest of the text became part of the plan. The plan now writes them as \u escapes. - A row's chosen action was looked up with its raw key in a CSS selector. A key with a quote made the plan build fail, and one with a backslash matched no control, so the plan silently used the default action. The key now goes through CSS.escape. Two problems remain. When two pairs write different rows to the same node and key, table-repair keeps whichever pair it handles last, in Go map order; that needs a fix in table-repair. And a plan still cannot name a node pair, so with many nodes a truncated report can leave more shown rows out of its plan.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe HTML report now identifies shown rows that share a key and difference type with unshown rows in another node pair. Repair-plan generation uses pair-aware selection and action data, omits unsafe rules, reports omitted rows, and escapes YAML-sensitive keys. ChangesHTML repair-plan generation
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No merge-blocking issue was established for the generated HTML repair plans; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change prevents several ways a generated plan could repair rows the operator did not choose. One multi-node case still needs a safeguard: different selected actions for the same key can produce conflicting writes to a shared node. Applying a repair plan requires an authorized operator, and the final outcome of that conflict has not been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
A rabbit checks the rows in pairs, Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 3 medium |
🟢 Metrics 31 complexity · 0 duplication
Metric Results Complexity 31 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
danolivo
left a comment
danolivo
left a comment
There was a problem hiding this comment.
Yeah, my Claude also wrote the same code at first. I removed it by hand because it doesn't solve the problem in general, and a general solution needs more time and effort. Below is why I reverted this approach in the previous PR. Maybe it will help somehow:
1. Omitted rows are not visible on the page
A row can be omitted from the plan for three reasons: it has a hidden twin, it is selected in only some pairs, or it has different actions in different pairs. In all three cases the page still shows the row with an active action control, and the user learns about the omission only from the YAML comment, which lists at most 20 rows. So a user can choose an action for a row, download the plan, and never notice that the row has no rule.
Suggestions:
hidden_twinis known when the page is rendered. The template could mark these rows ("Not in the repair plan: the same key is hidden in another pair") and disable their action control.- The other two reasons depend on user choices, so the page script could show a short notice when it builds the plan, for example "3 selected rows have no rule; see the comment at the top of the plan", before the copy or download happens.
2. Conflicting writes to the same node and key
TestHTMLPlanThreeNodesSameKeyInSeveralPairs accepts that key 5 goes to n1 from n2 in n1/n2 and from n3 in n1/n3, and that the result depends on Go map order. So the plan can produce a non-deterministic repair, and nothing tells the user. I think the plan should not create this situation silently.
The script can detect it: two entries for the same key whose actions write to the same node through different pairs (for example apply_from n2 insert for missing_on_n1 in both n1/n2 and n1/n3). Such entries could be omitted and listed in the comment with a reason such as "writes to the same node from different pairs". A check in table-repair that rejects two different writes to one node and key would be a good second guard, but that can be a separate change. When this is fixed, the test could check the
exact rows instead of the keys only.
3. The root cause stays
Hidden twins exist because the row limit is applied to each pair on its own. If the report chose its rows by key across all pairs (for example, take the union of the first N keys of each pair, then show every row of each chosen key in every pair), a shown key would always be shown in all its pairs, and the hidden_twin case could not happen. I do not ask for this in this PR, but it may be worth a follow-up issue, since the PR description already says that with many nodes the plan can omit many shown rows.
|
Handled the smaller bug fixes in a separate PR, can revisit the 3+ node case; closing. |
2 participants
A plan rule matches rows by primary key and diff_type in every node pair; it cannot name a pair. The page script did not allow for that:
The report now marks each shown row whose key and type the report hides in another pair (hidden_twin), counting rows with no visible difference, which the page never shows but table-repair still sees as row_mismatch. The script makes one entry per key and type, reads each row's action from its own pair's section, and uses only rules when the report has more than one pair. It leaves out a row when a rule for it would change a row nobody chose: its twin is hidden, it is selected in only some pairs, or it has a different action in another pair. The YAML lists those rows in a comment, and default_action is skip.
Older bugs in the same code are fixed as well:
Two problems remain. When two pairs write different rows to the same node and key, table-repair keeps whichever pair it handles last, in Go map order; that needs a fix in table-repair. And a plan still cannot name a node pair, so with many nodes a truncated report can leave more shown rows out of its plan.