Add Reconciliation Assistant skill - #286
Open
JagmeetChabraMSFT (jchha001) wants to merge 17 commits into
Open
Conversation
A domain-agnostic two-source reconciliation skill: config-driven tiered matching (exact, difference, similarity, grouped, timing) with a mandatory tie-out, delivered as a formula-driven Excel workbook (Dashboard + Reconciliation + source tabs) plus an optional styled HTML dashboard from the same computation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces the “Reconciliation Assistant” skill with a config-driven Python implementation plus supporting methodology/platform documentation, example configuration, and metadata to enable consistent reconciliation outputs across code-capable and non-code hosts.
Changes:
- Added a full
scripts/reconcile.pyreference implementation that loads two sources, performs tiered matching, generates a formula-driven Excel workbook, and optionally emits an HTML dashboard. - Added supporting docs (
SKILL.md, methodology + platform notes) and an example JSON config to define inputs, normalization, and matching behavior. - Added package metadata and user-facing README for skill discovery and usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| submissions/reconciliation-assistant/scripts/reconcile.py | Core reconciliation engine + Excel/HTML report generation. |
| submissions/reconciliation-assistant/references/platform-notes.md | Explains how to run the skill on Cowork/Scout vs Copilot Studio. |
| submissions/reconciliation-assistant/references/methodology.md | Defines reconciliation tiers, tie-out identity, and examples. |
| submissions/reconciliation-assistant/assets/config.example.json | Example config schema for sources/matching/output. |
| submissions/reconciliation-assistant/SKILL.md | Skill contract, guardrails, and reporting requirements. |
| submissions/reconciliation-assistant/README.md | User-facing overview and expected outputs. |
| submissions/reconciliation-assistant/metadata.json | Skill metadata (name, version, dates, tags). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+108
to
+116
| def reconcile(df_a, df_b, config): | ||
| src = config["sources"] | ||
| m = config["matching"] | ||
| norm = config.get("normalization", {}) | ||
| abs_tol = m.get("amountToleranceAbsolute", 0.01) | ||
| pct_tol = m.get("amountTolerancePercent", 0.0) | ||
|
|
||
| a_keys = src["a"]["keyColumns"] | ||
| b_keys = src["b"]["keyColumns"] |
Comment on lines
+165
to
+174
| candidates.sort(key=lambda r: abs((r["_amt"] or 0) - (ra["_amt"] or 0))) | ||
| rb = candidates[0] | ||
| used_b.add(rb["_idx"]) | ||
| ra["_matched"] = True | ||
| if within_tolerance(ra["_amt"], rb["_amt"], abs_tol, pct_tol): | ||
| status = "Matched" | ||
| diff = 0.0 | ||
| else: | ||
| status = "Matched (with difference)" | ||
| diff = (ra["_amt"] or 0) - (rb["_amt"] or 0) |
| continue | ||
| except Exception: | ||
| pass | ||
| sim = similarity(ra["_key"], rb["_key"]) |
Comment on lines
+1068
to
+1070
| la = config["sources"]["a"].get("label", "Source A") | ||
| lb = config["sources"]["b"].get("label", "Source B") | ||
| sa, sb = la[:31], lb[:31] |
Comment on lines
+1083
to
+1084
| ws_sa = wb.create_sheet(sa) | ||
| ws_sb = wb.create_sheet(sb if sb != sa else sb + " (B)") |
Comment on lines
+1555
to
+1556
| p.add_argument("--sheet-a", default=None, help="Sheet name/index for source A (for workbook tabs)") | ||
| p.add_argument("--sheet-b", default=None, help="Sheet name/index for source B (for workbook tabs)") |
Comment on lines
+1570
to
+1571
| df_a = load_table(args.source_a, sheet_a) | ||
| df_b = load_table(args.source_b, sheet_b) |
Comment on lines
+275
to
+281
| # Tier 4b: timing differences. Among the still-unmatched records, detect the classic | ||
| # "same item posted to a different period" case: an A record and a B record sharing the | ||
| # reduced key (identity minus the period) and the same amount, but a different period. | ||
| # We ANNOTATE both lines (so they remain visible as one-sided breaks and count toward the | ||
| # variance the way an accountant expects) rather than collapsing them - the note preserves | ||
| # the timing insight for the reviewer. | ||
| if enable_timing: |
Comment on lines
+297
to
+304
| for ra in unmatched_a: | ||
| results.append({"status": "Unmatched (A)", "a_idx": ra["_idx"], "b_idx": None, | ||
| "key": ra["_key"], "amount_a": ra["_amt"], "amount_b": None, | ||
| "difference": None, "evidence": ra.get("_timing_note", "")}) | ||
| for rb in unmatched_b: | ||
| results.append({"status": "Unmatched (B)", "a_idx": None, "b_idx": rb["_idx"], | ||
| "key": rb["_key"], "amount_a": None, "amount_b": rb["_amt"], | ||
| "difference": None, "evidence": rb.get("_timing_note", "")}) |
Comment on lines
+309
to
+315
| def tie_out(results, total_a, total_b, abs_tol): | ||
| diffs = sum(r["difference"] for r in results | ||
| if r["status"] == "Matched (with difference)" and r["difference"] is not None) | ||
| unm_a = sum(r["amount_a"] for r in results if r["status"] == "Unmatched (A)" and r["amount_a"] is not None) | ||
| unm_b = sum(r["amount_b"] for r in results if r["status"] == "Unmatched (B)" and r["amount_b"] is not None) | ||
| left = total_a - total_b | ||
| right = diffs + unm_a - unm_b |
- Use matching.keyMap to align differently-named key columns across sources. - Do not fabricate a variance when an exact-key pair has a missing amount (route to Needs Review); include probable/grouped deltas in the tie-out identity. - Guard similarity matching against empty keys; apply signConvention consistently in the HTML path and the source-tab amounts so all outputs agree. - Sanitize/So dedupe Excel sheet names (<=31 chars, reserved chars); neutralize spreadsheet formula injection in source-tab text; escape HTML sub-header. - Vectorize column-width autofit; prune the grouped-match search (same-sign, magnitude, attempt cap); parse digit-only --sheet-a/-b as an index. - Align docs: timing is an annotation on the two Unmatched lines (Root Cause=Timing), not a distinct state; document same-sign grouping. Remove dead helpers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Suppressed comments (4)
submissions/reconciliation-assistant/scripts/reconcile.py:781
- This loop does repeated linear searches (
desc_cols.index,meta_a['cols'].index,b_cols.index) inside the per-row write path, yielding O(n_lines * D^2) behavior for large sheets. Precompute{name: col_idx}and{col_name: letter}maps once outside the loop to keep report generation fast on large datasets.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
if side == "a":
src_letter = _CL(meta_a["cols"].index(name) + 1)
ref = f"='{sa}'!${src_letter}{srow}"
else:
bname = keymap.get(name, name)
if bname in b_cols:
src_letter = _CL(b_cols.index(bname) + 1)
ref = f"='{sb}'!${src_letter}{srow}"
submissions/reconciliation-assistant/scripts/reconcile.py:1165
- The post-processing font pass iterates every cell in every sheet, which can become a major bottleneck (and memory/time risk) for tens of thousands of rows. Prefer setting fonts at write-time (or applying a NamedStyle / default style where possible), or restrict the pass to the known written ranges rather than
iter_rows()over entire worksheets.
for ws in wb.worksheets:
for row in ws.iter_rows():
for c in row:
if c.value is None or c.font.name == "Arial":
continue
f = c.font
c.font = Font(name=REPORT_FONT, size=f.size, bold=f.bold,
italic=f.italic, color=f.color)
submissions/reconciliation-assistant/assets/config.example.json:23
matching.amountMatchappears in the example config but is not used byscripts/reconcile.py(the script always uses tolerance fields and does not branch on exact vs tolerance mode). This can confuse users and lead to incorrect expectations. Either implement behavior that honorsamountMatch(e.g., force zero tolerances whenexact) or remove it from the config example/schema to keep configuration truthful.
"amountMatch": "exact",
"amountToleranceAbsolute": 0.01,
"amountTolerancePercent": 0.0,
submissions/reconciliation-assistant/scripts/reconcile.py:274
- The comment says combinations are searched ‘nearest-magnitude-first’, but
reverse=Truesorts by descending magnitude (largest first). Update the comment or the sort order so the documentation matches the actual behavior.
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
Comment on lines
+658
to
+659
| return " | ".join(str(df.iloc[i][k]) for k in keys) | ||
| a_set = {keystr(df_a, a_keys, i).lower() for i in range(meta_a["n"])} |
Comment on lines
+1194
to
+1198
| gk = tuple(str(field(r["key"], k)) for k in nontiming) | ||
| grp.setdefault(gk, []).append(r) | ||
| for r in rows: | ||
| if r["status"] == "Reconciled": | ||
| r["rootcause"] = "—" |
Comment on lines
+1213
to
+1214
|
|
||
|
|
Comment on lines
+1219
to
+1220
|
|
||
| def build_html_dashboard(rows, config, src_name=None): |
Comment on lines
+141
to
+145
| r["_tval"] = norm_key(r.get(a_keys[a_timing_idx]), norm) | ||
| for j, r in enumerate(b): | ||
| r["_idx"] = j | ||
| r["_key"] = build_key(r, b_keys, norm) | ||
| r["_amt"] = apply_sign(normalize_amount(r.get(b_amt_col), norm), src["b"].get("signConvention", "asIs")) |
Comment on lines
+164
to
+166
| continue | ||
| candidates.sort(key=lambda r: abs((r["_amt"] or 0) - (ra["_amt"] or 0))) | ||
| rb = candidates[0] |
|
|
||
|
|
||
| def build_key(row, key_cols, norm): | ||
| return "||".join(norm_key(row.get(c), norm) for c in key_cols) |
Comment on lines
+179
to
+180
| unmatched_a = [r for r in a if not r.get("_matched")] | ||
| unmatched_b = [r for r in b if r["_idx"] not in used_b] |
Comment on lines
+217
to
+220
|
|
||
| # Tier 4: grouped (split / partial) matches. One record on one side equals the sum of | ||
| # several on the other within tolerance (e.g. one invoice settled by three payments). | ||
| # Bounded for safety: enumeration is skipped when the opposite pool is too large, and |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Suppressed comments (2)
submissions/reconciliation-assistant/scripts/reconcile.py:131
- The config schema includes
matching.amountMatch(e.g.,exactvstolerance), but the implementation appears to ignore it and always uses the tolerance values (defaulting to 0.01/0.0). This makes the config misleading and can surprise users who setamountMatch: \"exact\"expecting stricter behavior (e.g., equality after rounding to cents). Consider either implementingamountMatch(e.g., set tolerances to 0 / compare rounded values whenexact) or removing the field from the schema/docs to avoid an API contract that isn’t honored.
m = config["matching"]
norm = config.get("normalization", {})
abs_tol = m.get("amountToleranceAbsolute", 0.01)
pct_tol = m.get("amountTolerancePercent", 0.0)
submissions/reconciliation-assistant/scripts/reconcile.py:773
- Inside the row loop, computing
desc_cols.index(name)makes this O(rows * cols²) for the reconciliation sheet. Precomputing a{name: col_idx}mapping (or iterating withenumerate(desc_cols)) avoids repeated linear searches and reduces overhead noticeably on wide datasets.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
if side == "a":
Comment on lines
+641
to
+642
| text_cols = {name for name in cols if not pd.api.types.is_numeric_dtype(df[name])} | ||
| for r, (_, row) in enumerate(df.iterrows(), start=2): |
Comment on lines
+658
to
+661
| # Matching Key formula: join the key cells with " | ". | ||
| joined = '&" | "&'.join(f"${kl}{r}" for kl in meta["key_letters"]) | ||
| mkc = ws.cell(row=r, column=mk_c, value=f"={joined}") | ||
| mkc.font = Font(color=MK_C) |
Comment on lines
+232
to
+235
| if abs((pd.to_datetime(da) - pd.to_datetime(db)).days) > window: | ||
| continue | ||
| except Exception: | ||
| pass |
Comment on lines
+655
to
+660
| cell = ws.cell(row=r, column=c, value=v) | ||
| cell.number_format = "0" | ||
| cell.alignment = Alignment(horizontal="left") | ||
| # Matching Key formula: join the key cells with " | ". | ||
| joined = '&" | "&'.join(f"${kl}{r}" for kl in meta["key_letters"]) | ||
| mkc = ws.cell(row=r, column=mk_c, value=f"={joined}") |
Comment on lines
+1129
to
+1131
| res_df = pd.DataFrame(results) | ||
| counts = res_df["status"].value_counts().to_dict() if not res_df.empty else {} | ||
| narrative = _build_narrative(summary, counts, config, results, df_a, df_b) |
| _write_source_tab(ws_sa, df_a, meta_a, sa, sign=sign_a, norm=norm) | ||
| _write_source_tab(ws_sb, df_b, meta_b, sb, sign=sign_b, norm=norm) | ||
| info = _write_reconciliation(ws_recon, df_a, df_b, config, meta_a, meta_b, sa, sb) | ||
| narr_rows = _write_dashboard(ws_dash, info, config, df_a, df_b, meta_a, meta_b, sa, sb, narrative, src_name) |
Address second round of review feedback on reconciliation-assistant: - norm_key: canonicalize integer-valued floats (7100.0 -> "7100") and empty/NaN to "" so Python keys match Excel's text concatenation. - build_key: return "" when all key components are empty instead of a separator-only string. - Excel Matching Key: new _xl_key_formula wraps each key cell in TRIM()/LOWER() per the normalization config, applied identically in the source tabs and the Reconciliation sheet so SUMIF/COUNTIF bind. Drop the forced "0" number format on non-amount key cells. - HTML/union keys: keystr and compute_reconciliation aggregate via norm_key per component so the dashboard groups keys the same way. - Timing detection: disable when the period column is absent from A's key or its aligned position is out of range for B's key (guards an IndexError). - Similarity date window: use total_seconds()/86400 for sub-day precision and skip a pair whose dates cannot be parsed instead of relaxing the rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Suppressed comments (4)
submissions/reconciliation-assistant/scripts/reconcile.py:208
- The new status assignment uses
\"Probable (Needs Review)\"for an exact-key match where an amount is missing. Inreferences/methodology.mdandSKILL.md, “Probable” is defined specifically as the similarity-matching outcome (Tier 3), so this reuse will confuse rollups and any consumer relying on the documented states. Consider introducing a distinct status (e.g.,\"Exact key (Needs Review)\"/\"Matched (Needs Review)\") and updating downstream summaries (including tie-out classification) and documentation accordingly.
if ra["_amt"] is None or rb["_amt"] is None:
# Key matches on both sides but an amount is blank/unparseable. Do not silently
# invent a clean variance; flag for review. The difference kept here is the
# balancing contribution to the tie-out identity (a blank amount contributed 0
# to its control total), not a fabricated "matched" number.
status = "Probable (Needs Review)"
diff = (ra["_amt"] or 0) - (rb["_amt"] or 0)
evidence = "exact key; amount missing on one side - verify before treating as matched"
submissions/reconciliation-assistant/scripts/reconcile.py:690
- There are hot-loop patterns that will scale poorly on large inputs:
df.iterrows()is slow for row-by-row extraction, anddesc_cols.index(name)/meta_a['cols'].index(name)do linear scans inside the per-row/per-column loops. Precompute name→index/letter maps once (outside the row loop) and consider switching toitertuples()for the source-tab write to reduce Python overhead. This will materially improve runtime for larger ledgers (which the docs claim are supported).
for r, (_, row) in enumerate(df.iterrows(), start=2):
submissions/reconciliation-assistant/scripts/reconcile.py:816
- There are hot-loop patterns that will scale poorly on large inputs:
df.iterrows()is slow for row-by-row extraction, anddesc_cols.index(name)/meta_a['cols'].index(name)do linear scans inside the per-row/per-column loops. Precompute name→index/letter maps once (outside the row loop) and consider switching toitertuples()for the source-tab write to reduce Python overhead. This will materially improve runtime for larger ledgers (which the docs claim are supported).
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
if side == "a":
src_letter = _CL(meta_a["cols"].index(name) + 1)
submissions/reconciliation-assistant/scripts/reconcile.py:427
- The report formatting comments state “No currency symbol” for amounts, but
_money()hardcodes$for narrative/headline strings. If the tool is intended to be currency-agnostic (andexpectedCurrencyis configurable), consider either removing the symbol or deriving it from config so the narrative doesn’t contradict the workbook conventions/documentation.
def _money(x):
"""Accounting-style: negatives in parentheses, whole dollars."""
x = x or 0
return f"$({abs(x):,.0f})" if x < 0 else f"${x:,.0f}"
Comment on lines
+95
to
+101
| def build_key(row, key_cols, norm): | ||
| parts = [norm_key(row.get(c), norm) for c in key_cols] | ||
| # If every component is empty there is no usable key: return "" so Tier 1 (exact) and Tier 3 | ||
| # (similarity) treat it as keyless rather than matching on a "||"-only string. | ||
| if not any(parts): | ||
| return "" | ||
| return "||".join(parts) |
| return v | ||
|
|
||
|
|
||
| def _xl_key_formula(cell_refs, norm): |
| if lower: | ||
| expr = f"LOWER({expr})" | ||
| parts.append(expr) | ||
| return "=" + '&" | "&'.join(parts) |
Comment on lines
+756
to
+757
| def keystr(df, keys, i): | ||
| return " | ".join(norm_key(df.iloc[i][k], norm) for k in keys) |
| # Canonical key: norm_key per component (trim/case/int-canonicalization) joined the same | ||
| # way the Python matcher's build_key does, so the HTML groups keys identically to the | ||
| # workbook and to reconcile(). | ||
| return "||".join(norm_key(row.get(k), norm) for k in keys) |
Third review round on reconciliation-assistant, plus an independent full-file audit so the outputs are internally consistent by construction: - Single key delimiter (KEY_DELIM) and a join_key_parts() helper shared by the Python matcher (build_key/reduced_key), the workbook union (keystr) and the HTML path (kstr); all collapse an all-empty key to "". _xl_key_formula mirrors this in Excel: TRIM/LOWER per config, KEY_DELIM, and an IF() that collapses an all-empty key to "" - so the source-tab helper and the Reconciliation sheet bind. - norm_key now collapses internal space runs and trims spaces to mirror Excel TRIM(), so the Python union grouping matches the workbook's TRIM'd keys and SUMIF/COUNTIF cannot double-count values differing only by internal whitespace. - The Dashboard headlines are now built from the per-key reconciliation model (compute_reconciliation) - the same model the Reconciliation sheet and the HTML dashboard use - via a shared _build_narrative_perkey(). Previously they came from the tiered record matcher and could disagree with the sheet totals (e.g. 28 tiered lines vs 27 union rows). The narrative now carries no currency symbol. - Removed the now-dead tiered-narrative helpers (_money, _account_facts, _account_nature, _bucket_facts, _build_narrative), the _dol alias, unused constants and three dead locals. Verified: GL and WHT regenerated and formula-evaluated (differences tie to -235,500.00 and -18,874.70, controls read OK); Excel and HTML headlines are now identical and agree with the sheet totals; npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
submissions/reconciliation-assistant/scripts/reconcile.py:1254
- The Excel dashboard path filters
output.groupBydown to columns that exist insources.a.keyColumns, but the HTML path usesgroupByas-is. This can cause the HTML dashboard’s basis line, "Difference by company/period" table, and headlines to diverge from the workbook whengroupByincludes non-key columns. Recommendation (mandatory): apply the same filtering in the HTML path (either incompute_reconciliationand/orbuild_html_dashboard) so both outputs use identical grouping dimensions.
def build_html_dashboard(rows, config, src_name=None):
import html as _h
la = config["sources"]["a"].get("label", "Source A")
lb = config["sources"]["b"].get("label", "Source B")
out = config.get("output", {})
group_by = out.get("groupBy", [])
submissions/reconciliation-assistant/scripts/reconcile.py:729
desc_cols.index(name)inside the loop makes this O(n^2) over description columns for every reconciliation row, which can become noticeable on large sheets. Recommendation (optional but strongly suggested): iterate withfor i, name in enumerate(desc_cols):and computecol_idx = 2 + i, or precompute a{name: index}mapping once.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
submissions/reconciliation-assistant/scripts/reconcile.py:570
sheet_titleis not used inside_write_source_tab, which makes the function signature misleading and increases call-site noise. Recommendation (optional): remove the parameter (and update callers), or use it for something concrete (e.g., writing a title cell or validatingws.title).
def _write_source_tab(ws, df, meta, sheet_title, sign="asIs", norm=None):
Comment on lines
+163
to
+164
| abs_tol = m.get("amountToleranceAbsolute", 0.01) | ||
| pct_tol = m.get("amountTolerancePercent", 0.0) |
Comment on lines
+984
to
+1013
| section(21, 1, "Difference by account") | ||
| for j, h in enumerate([acct_hdr, name_hdr, f"Amount — {la}", f"Amount — {lb}", "Difference"]): | ||
| header(22, 1 + j, h) | ||
| # Unique accounts in order of appearance. | ||
| accounts = [] | ||
| seen = set() | ||
| keymap = dict(zip(config["sources"]["a"]["keyColumns"], config["sources"]["b"]["keyColumns"])) | ||
| for side, srow in info["recon_rows"]: | ||
| if side == "a": | ||
| acct = df_a.iloc[srow - 2][acct_col] if acct_col else None | ||
| nm = df_a.iloc[srow - 2][name_col] if name_col else "" | ||
| else: | ||
| bacct = keymap.get(acct_col, acct_col) | ||
| acct = df_b.iloc[srow - 2][bacct] if (acct_col and bacct in df_b.columns) else None | ||
| nm = df_b.iloc[srow - 2][name_col] if (name_col and name_col in df_b.columns) else "" | ||
| if acct is not None and acct not in seen: | ||
| seen.add(acct); accounts.append((acct, nm)) | ||
| acc_start = 23 | ||
| for i, (acct, nm) in enumerate(accounts): | ||
| row = acc_start + i | ||
| ws.cell(row=row, column=1, value=acct).alignment = Alignment(horizontal="left") | ||
| txt(ws.cell(row=row, column=2, value=nm)) | ||
| ws.cell(row=row, column=3, value=f"=SUMIF({R(RC)},$A{row},{R(Fa)})").number_format = ACCT2 | ||
| ws.cell(row=row, column=4, value=f"=SUMIF({R(RC)},$A{row},{R(Fb)})").number_format = ACCT2 | ||
| ws.cell(row=row, column=5, value=f"=C{row}-D{row}").number_format = ACCT2 | ||
| acc_tot = acc_start + len(accounts) | ||
| ws.cell(row=acc_tot, column=2, value="Total").font = Font(bold=True) | ||
| for col, base in ((3, "C"), (4, "D"), (5, "E")): | ||
| cc = ws.cell(row=acc_tot, column=col, value=f"=SUM({base}{acc_start}:{base}{acc_tot-1})") | ||
| cc.font = Font(bold=True); cc.number_format = ACCT2 |
…ections Fourth review round on reconciliation-assistant, plus an independent full-file audit: - Honor matching.amountMatch. A new effective_tolerances() returns zero tolerance in "exact" mode (amounts must agree to the cent, regardless of any tolerance values left in the config) and the configured absolute/percent tolerances in "tolerance" mode. Used by reconcile() and the tie-out. The example config and methodology.md are updated to match (exact -> 0 tolerance). - The Reconciliation sheet is now genuinely one row per unique key: recon_rows dedups source-A rows by key (first-seen) before adding new B keys, mirroring compute_reconciliation(). Previously it emitted one row per source-A row, so a key duplicated within a source produced two rows whose SUMIFs each returned the full key-group sum - double-counting the totals and diverging from the HTML. The workbook and HTML now agree even when a source repeats a key. - Guard the optional Dashboard blocks. "Difference by account" is only drawn when the account column is configured, resolvable to a Reconciliation column, and present in the data; "Difference by company and period" only when both group dimensions resolve and at least one combo exists. This avoids SUMIF ranges over a non-existent column and reversed SUM() ranges when a section is empty. - Removed the unused output.format / output.fileName keys from the example config (nothing reads them). Verified: GL and WHT regenerated and formula-evaluated (differences tie to -235,500.00 and -18,874.70, controls read OK); a duplicate-key case now ties the sheet to the HTML; exact mode rejects a 0.005 difference while tolerance mode accepts it; npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
submissions/reconciliation-assistant/scripts/reconcile.py:1617
_parse_sheettreats negative numeric strings (e.g. "-1") as valid sheet indices, which can lead to unexpected behavior or runtime errors inpandas.read_excel(sheet_name=...)(sheet indexes are typically non-negative). Consider only accepting non-negative digit strings (e.g.s.strip().isdigit()), or explicitly validating the integer is>= 0before converting.
def _parse_sheet(s):
if isinstance(s, str) and s.strip().lstrip("-").isdigit():
return int(s.strip())
return s
submissions/reconciliation-assistant/scripts/reconcile.py:608
df.iterrows()is a known performance bottleneck for large DataFrames and can significantly slow workbook generation at the scales mentioned in the docs. Switching toitertuples(index=False, name=None)(or iterating overdf.to_numpy()alongsidecols) will be materially faster while preserving the same output.
for r, (_, row) in enumerate(df.iterrows(), start=2):
for c, name in enumerate(cols, start=1):
v = row[name]
submissions/reconciliation-assistant/scripts/reconcile.py:749
- Inside the main
recon_rowsloop,desc_cols.index(name)makes this O(rows * cols^2) due to repeated linear searches. Useenumerate(desc_cols)to computecol_idxdirectly (and avoid recomputing indexes) to reduce overhead when there are many columns.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
submissions/reconciliation-assistant/scripts/reconcile.py:1650
- The comment/error text is a bit ambiguous because the script does compute a record-to-record tie-out identity (
tie_out(...)) later, but it does not implement control-total mode. Consider rewording to explicitly say “control-total tie-out mode (Step 3b) is not implemented by this script” to avoid confusing users into thinking no tie-out is performed at all.
# This reference script implements the record-to-record tiered match. Control-total
# tie-out (SKILL.md Step 3b) is an analytical method the agent performs directly; the
# script does not run it, so refuse rather than silently emit record-to-record output.
mode = config.get("matching", {}).get("reconciliationMode", "recordToRecord")
if mode and mode != "recordToRecord":
sys.exit(f"reconciliationMode '{mode}' is not run by this script. It implements "
"record-to-record matching only; perform control-total tie-out analytically "
"per SKILL.md Step 3b, or set matching.reconciliationMode to 'recordToRecord'.")
Comment on lines
+183
to
+190
| timing_col = m.get("timingKeyColumn") | ||
| enable_timing = m.get("enableTimingDetection", True) and timing_col is not None | ||
| a_timing_idx = a_keys.index(timing_col) if (enable_timing and timing_col in a_keys) else None | ||
| # Disable timing unless the period column is present in A's key AND the aligned B key has a | ||
| # column at the same position (guards against an IndexError / wrong reduced key when B's | ||
| # keyColumns are shorter or were not aligned to A via keyMap). | ||
| if enable_timing and (a_timing_idx is None or a_timing_idx >= len(b_keys)): | ||
| enable_timing = False |
Comment on lines
+559
to
+561
| bare = "&".join(parts) # components with no delimiter, for the empty test | ||
| joined = ('&"' + KEY_DELIM + '"&').join(parts) | ||
| return f'=IF({bare}="","",{joined})' |
Fifth review round on reconciliation-assistant, plus an independent full-file audit: - Timing detection is now guarded consistently across the tiered matcher, the formula workbook, and the HTML per-key model. It only applies when timing detection is enabled, a timing column is set, AND there is at least one non-timing key column, and it is skipped for any row whose reduced (non-timing) key is blank. Previously, when the timing column was the only key column - or a row's non-timing components were all blank - the reduced key collapsed to "" and unrelated one-sided breaks could be paired/labelled as timing differences. - Keyless rows (every key component blank) are no longer collapsed into a single union key. Each keyless row now gets a unique placeholder key (written literally and identically into the source tab helper and the Reconciliation Matching Key, so SUMIF/COUNTIF still bind), so keyless rows surface as individual one-sided breaks - matching the record matcher, which treats an empty key as non-matchable - instead of being netted together into a false "reconciled" line. - _safe_sheet_name also strips "~" (a SUMIF/COUNTIF wildcard-escape) so a sheet name embedded in a keyless row's literal key can't distort the criteria. - methodology.md documents both the timing guard and the keyless-row handling. Verified: GL and WHT regenerated and formula-evaluated - differences tie to -235,500.00 and -18,874.70, controls read OK, and their legitimate timing root causes (2 and 4 items) are unchanged; keyless rows now surface as separate one-sided breaks in both the workbook and the HTML and are never labelled Timing; timing-only-key configs produce no Timing anywhere; npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
submissions/reconciliation-assistant/scripts/reconcile.py:1424
- The HTML control panel is currently misleading in two ways: (1) the first three controls hardcode ok=True and set result==expected from the same value, so they can never fail and do not validate anything; (2) the status pill always uses class 'ok' even when text is 'CHECK', so failures won’t be visually distinguishable. Recommend either computing meaningful checks from independently-derived values (e.g., pass in raw source totals/unique-key counts) or removing controls you can’t validate in HTML, and also conditionally applying a distinct CSS class for CHECK to avoid presenting failed controls as OK-styled.
ctrls = [
("Every key in either ledger appears once", str(total), str(total), True),
(f"Amount — {la} agrees to the {la} tab", _num(total_a), _num(total_a), True),
(f"Amount — {lb} agrees to the {lb} tab", _num(total_b), _num(total_b), True),
("Total difference proves to the two ledger totals", "—", "—", round(net - (total_a - total_b), 2) == 0),
("Reconciled plus open items equal total lines", str(reconciled + opn), str(total), reconciled + opn == total),
]
ctrl_html = "".join(
f'<tr><td>{e(lbl)}</td><td class="num">{res}</td><td class="num">{exp}</td>'
f'<td><span class="ok">{"OK" if ok else "CHECK"}</span></td></tr>'
for lbl, res, exp, ok in ctrls)
submissions/reconciliation-assistant/scripts/reconcile.py:248
- For duplicate exact keys (multiple candidates on B for one A key, or vice versa), the implementation only tie-breaks by nearest amount; it does not incorporate the documented 'then nearest date' tie-breaker described in methodology.md ('matched by nearest amount then nearest date'). This can produce different pairings than the documented method when amounts are equal/close across candidates. Recommend extending the sort key to include date distance when both sources have date columns configured, so the record-to-record matcher aligns with the documented determinism rules for ambiguous duplicates.
# Tier 1 + 2: exact key
for ra in a:
candidates = [r for r in b_by_key.get(ra["_key"], []) if r["_idx"] not in used_b and ra["_key"] != ""]
if not candidates:
continue
candidates.sort(key=lambda r: abs((r["_amt"] or 0) - (ra["_amt"] or 0)))
submissions/reconciliation-assistant/scripts/reconcile.py:1212
- This post-processing pass iterates every cell in every worksheet to apply fonts, which can become a major runtime/memory cost for large ledgers (tens of thousands of rows × many columns). A more scalable approach is to set the desired font at write-time (cells you create) or use openpyxl NamedStyle/default styles per sheet/column, avoiding a full workbook traversal.
for ws in wb.worksheets:
for row in ws.iter_rows():
for c in row:
if c.value is None or c.font.name == "Arial":
continue
f = c.font
c.font = Font(name=REPORT_FONT, size=f.size, bold=f.bold,
italic=f.italic, color=f.color)
submissions/reconciliation-assistant/scripts/reconcile.py:803
- Inside the per-row write loop, using desc_cols.index(name) makes column lookup O(D) for every descriptor cell, which adds avoidable overhead for wide sheets. Recommend iterating with enumerate(desc_cols) (or precomputing a name->position dict) so col_idx is O(1) per cell.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
submissions/reconciliation-assistant/README.md:33
- This README description suggests timing differences are delivered as a 'Needs Review' state, but the methodology.md and reconcile.py implementation describe timing as an annotation on still-unmatched lines (they remain Unmatched (A)/(B), with timing notes / Root Cause = Timing in the workbook model). Recommend updating the README wording to match the implemented behavior to avoid confusing users about whether timing creates a new match state.
5. **Timing** — the same item posted to a *different period* on each side (same account and amount, different month) → flagged as a timing difference in Needs Review, instead of showing up as two confusing one-sided breaks. Ideal for GL, bank, and accrual reconciliations.
6. **Unmatched** — everything left, split by which side it came from.
submissions/reconciliation-assistant/scripts/reconcile.py:342
- The comment says 'Search smaller (nearest-magnitude-first)' but the code sorts by abs(amount) with reverse=True (largest-magnitude-first). Either the comment is incorrect or the sort direction is. Recommend making them consistent (update the comment if largest-first is intended for pruning, or change the sort order if nearest-first is intended).
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
| controls = [ | ||
| ("Every key in either ledger appears once", | ||
| f"=COUNTA(Reconciliation!$A${rf}:$A${rl})", | ||
| f"=COUNTA({a_mk})+SUMPRODUCT(--(COUNTIF({a_mk},{b_mk})=0))", |
Sixth review round on reconciliation-assistant. The Dashboard control "Every key in either ledger appears once" compared the Reconciliation row count (one row per unique union key) against an expected value built with COUNTA/COUNTIF over the source helper ranges, which counts raw rows. Once the union de-duplicates keys within a source, that expected value overcounts whenever a source repeats a key, so the control read CHECK on a correct reconciliation. The expected value is now the actual union size - unique keys in A plus unique keys in B not in A - using the 1/COUNTIF unique-count pattern: =SUMPRODUCT(1/COUNTIF(a_mk,a_mk)) +SUMPRODUCT((COUNTIF(a_mk,b_mk)=0)/COUNTIF(b_mk,b_mk)) Each keyless row carries a distinct placeholder key, so it counts once apiece. Each term drops to 0 when its source has no data rows, so the helper range never inverts to include a blank cell (which would make 1/COUNTIF raise #DIV/0!). Verified by formula evaluation: with intra-source duplicate keys the control now reads OK (result == expected == unique union count); an empty source on either side reads OK with no #DIV/0!; GL and WHT still tie out with all five controls OK; npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
submissions/reconciliation-assistant/scripts/reconcile.py:450
tie_out()floors the tie-out threshold to0.01even when the configured absolute tolerance is0.0(exact mode). That can mark a reconciliation as tied out when it is not (up to 1 cent residual). Suggest using the configured tolerance as-is (and, if needed, adding only a tiny floating-point epsilon like1e-9), or passing in a currency precision setting and applying the same quantization approach used for exact matching.
left = total_a - total_b
closed = abs(left - explained) <= max(abs_tol, 0.01)
return {"total_a": total_a, "total_b": total_b, "net_difference": left,
"explained": explained, "residual": left - explained, "tied_out": closed}
submissions/reconciliation-assistant/scripts/reconcile.py:1184
write_report()declaresdf_a/df_bas optional (Noneby default) but immediately passes them intocompute_reconciliation()and later_src_meta(), which will fail if either isNone. Either makedf_a/df_brequired parameters (preferred, since the workbook needs the source data), or handle theNonecase explicitly (e.g., skip per-key narrative/workbook generation and raise a clear error).
def write_report(results, config, out_path, df_a=None, df_b=None, src_name=None):
import openpyxl
la = config["sources"]["a"].get("label", "Source A")
lb = config["sources"]["b"].get("label", "Source B")
res_df = pd.DataFrame(results)
counts = res_df["status"].value_counts().to_dict() if not res_df.empty else {}
# The Dashboard headlines are built from the per-key reconciliation model (compute_reconciliation) -
# the same model that drives the Reconciliation sheet's formulas and the HTML dashboard - so the
# narrative counts can never disagree with the sheet totals. (The tiered `results`/`counts` are a
# record-level view returned for the caller's console summary, not the per-key artifact.)
perkey_rows, _, _ = compute_reconciliation(df_a, df_b, config)
submissions/reconciliation-assistant/scripts/reconcile.py:342
- The comment says “Search smaller (nearest-magnitude-first)”, but
reverse=Truesorts by largest magnitude first. Sinceitertools.combinations()enumerates in input order, the current code searches larger-magnitude members earlier. Update the comment to match the implemented behavior (or change the sort if the comment is what was intended) to avoid confusion when tuning grouped matching.
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
submissions/reconciliation-assistant/scripts/reconcile.py:804
desc_cols.index(name)inside the loop makes this O(D²) per reconciliation row (and it also re-scansdesc_colsrepeatedly). Precompute a{name: col_idx}mapping once (similar todesc_letter) and reference it here to keep worksheet generation predictable on wide datasets.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
if side == "a":
Comment on lines
+146
to
+164
| def within_tolerance(x, y, abs_tol, pct_tol): | ||
| if x is None or y is None: | ||
| return False | ||
| diff = abs(x - y) | ||
| if diff <= abs_tol: | ||
| return True | ||
| if pct_tol > 0 and max(abs(x), abs(y)) > 0: | ||
| return (diff / max(abs(x), abs(y))) * 100.0 <= pct_tol | ||
| return False | ||
|
|
||
|
|
||
| def effective_tolerances(matching): | ||
| """Resolve the amount-match tolerances honoring matching.amountMatch. In 'exact' mode the | ||
| amounts must agree exactly (to the cent for 2-dp currency data), so BOTH tolerances are 0 | ||
| regardless of any amountToleranceAbsolute/Percent left in the config; 'tolerance' mode (or an | ||
| unset amountMatch) uses the configured absolute/percent values (default 0.01 / 0).""" | ||
| if matching.get("amountMatch") == "exact": | ||
| return 0.0, 0.0 | ||
| return matching.get("amountToleranceAbsolute", 0.01), matching.get("amountTolerancePercent", 0.0) |
Comment on lines
+864
to
+869
| cl = ws.cell(row=r_ctrl, column=4, | ||
| value="Control — total difference proves to the two ledger totals (must be nil)") | ||
| cl.font = Font(color=SUB_C) | ||
| ctrl_cell = ws.cell(row=r_ctrl, column=_col_to_idx(L_diff) + 1, | ||
| value=f"={L_amt_a}{r_total}-{L_amt_b}{r_total}-{L_diff}{r_total}") | ||
| ctrl_cell.number_format = ACCT2 |
Seventh review round on reconciliation-assistant. - normalize_amount now rounds each parsed amount to 2 decimals (cents). With amountMatch "exact" resolving to a zero tolerance, binary floating-point noise could otherwise make two cells that should be equal (e.g. both "1250.00", or a 0.1+0.2 style value) differ by ~1e-16 and be pushed to "Matched (with difference)". Quantizing at normalization makes "exact to the cent" deterministic and keeps the matcher, the per-key model, and the workbook all comparing the same cent-rounded values. - The Reconciliation "must be nil" control was a tautology: amtA_total - amtB_total - diff_total, where diff = amtA - amtB per row, is identically 0, so it could never detect a dropped/duplicated key or a misaligned range. It now checks the per-key net difference against the difference of two INDEPENDENT source-tab column sums: diff_total - (SUM(source A amount) - SUM(source B amount)). - The HTML control panel was likewise comparing per-key totals to themselves. build_html_dashboard now receives the source frames and computes independent source totals directly from the raw records, so the Amount and Total-difference controls compare the per-key aggregation against an independent code path (they would read CHECK if aggregation dropped or double-counted a record). Verified: an FP-noisy equal pair (0.1+0.2 vs 0.3) now stays Matched; the Excel and HTML controls read OK on correct data via genuinely independent comparisons; GL and WHT still tie out (-235,500.00 / -18,874.70) with all five controls OK and unchanged counts; npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (8)
submissions/reconciliation-assistant/scripts/reconcile.py:1197
write_report()defaultsdf_a/df_btoNone, but unconditionally callscompute_reconciliation(df_a, df_b, config)and_src_meta(df_a, ...), which will raise at runtime if a caller uses the defaults. Makedf_a/df_brequired parameters (remove theNonedefaults), or add an explicit guard with a clear error explaining that report generation requires both dataframes.
def write_report(results, config, out_path, df_a=None, df_b=None, src_name=None):
import openpyxl
la = config["sources"]["a"].get("label", "Source A")
lb = config["sources"]["b"].get("label", "Source B")
res_df = pd.DataFrame(results)
counts = res_df["status"].value_counts().to_dict() if not res_df.empty else {}
# The Dashboard headlines are built from the per-key reconciliation model (compute_reconciliation) -
# the same model that drives the Reconciliation sheet's formulas and the HTML dashboard - so the
# narrative counts can never disagree with the sheet totals. (The tiered `results`/`counts` are a
# record-level view returned for the caller's console summary, not the per-key artifact.)
perkey_rows, _, _ = compute_reconciliation(df_a, df_b, config)
narrative = _build_narrative_perkey(perkey_rows, config)
meta_a = _src_meta(df_a, config["sources"]["a"], config)
meta_b = _src_meta(df_b, config["sources"]["b"], config)
submissions/reconciliation-assistant/scripts/reconcile.py:347
- The comment says “Search smaller (nearest-magnitude-first) combinations first”, but the implementation sorts
reverse=True(largest magnitude first). Either update the comment to match the intended behavior, or change the sort direction if the goal is actually nearest-magnitude-first.
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
submissions/reconciliation-assistant/scripts/reconcile.py:768
- Timing root-cause detection in the workbook compares the raw descriptive key columns for equality via
COUNTIFS. This can misclassify timing items as “Scope / mapping” when A and B values differ only by normalization rules you already apply elsewhere (TRIM/LOWER, integer-float canonicalization), because the union row’s descriptive columns can originate from different sources. To keep Excel root-cause logic consistent withnorm_key()/_xl_key_formula(), introduce a normalized “Reduced Key” helper column (non-timing key components joined with the same normalization) and use that single helper forCOUNTIFSinstead of raw column-by-column comparisons.
nontiming_keys = [k for k in a_keys if k != timing_col and k in desc_letter]
submissions/reconciliation-assistant/scripts/reconcile.py:849
- Timing root-cause detection in the workbook compares the raw descriptive key columns for equality via
COUNTIFS. This can misclassify timing items as “Scope / mapping” when A and B values differ only by normalization rules you already apply elsewhere (TRIM/LOWER, integer-float canonicalization), because the union row’s descriptive columns can originate from different sources. To keep Excel root-cause logic consistent withnorm_key()/_xl_key_formula(), introduce a normalized “Reduced Key” helper column (non-timing key components joined with the same normalization) and use that single helper forCOUNTIFSinstead of raw column-by-column comparisons.
if row_timing:
countifs = ""
for k in nontiming_keys:
kl = desc_letter[k]
countifs += f"${kl}$5:${kl}${r_last},${kl}{r},"
opp = f'IF({L_dtype}{r}="Missing in {lb}","Missing in {la}","Missing in {lb}")'
root = (f'=IF({L_status}{r}="Reconciled","—",'
f'IF({L_dtype}{r}="Amount mismatch","Measurement",'
f'IF(COUNTIFS({countifs}${L_dtype}$5:${L_dtype}${r_last},{opp})>0,'
f'"Timing","Scope / mapping")))')
submissions/reconciliation-assistant/scripts/reconcile.py:1340
- The HTML timing root-cause grouping key
gkis built from rawstr(field(...)), while reduced-key emptiness usesnorm_key(...). This makes timing detection sensitive to unnormalized differences (case/whitespace/float rendering) and can diverge from the matcher and Excel (which are intended to be normalization-driven). Buildgkusing the same normalized components you use elsewhere (e.g.,join_key_parts([norm_key(field(...), norm) ...])overnontiming) so offset detection matches the platform-determinism goal.
nontiming = [k for k in a_keys if k != timing_col]
timing_on = (config["matching"].get("enableTimingDetection", True)
and timing_col is not None and len(nontiming) >= 1)
grp = {}
for r in rows:
gk = tuple(str(field(r["key"], k)) for k in nontiming)
grp.setdefault(gk, []).append(r)
submissions/reconciliation-assistant/scripts/reconcile.py:1348
- The HTML timing root-cause grouping key
gkis built from rawstr(field(...)), while reduced-key emptiness usesnorm_key(...). This makes timing detection sensitive to unnormalized differences (case/whitespace/float rendering) and can diverge from the matcher and Excel (which are intended to be normalization-driven). Buildgkusing the same normalized components you use elsewhere (e.g.,join_key_parts([norm_key(field(...), norm) ...])overnontiming) so offset detection matches the platform-determinism goal.
gk = tuple(str(field(r["key"], k)) for k in nontiming)
opp = f"Missing in {la}" if r["difftype"] == f"Missing in {lb}" else f"Missing in {lb}"
submissions/reconciliation-assistant/scripts/reconcile.py:1356
- The HTML timing root-cause grouping key
gkis built from rawstr(field(...)), while reduced-key emptiness usesnorm_key(...). This makes timing detection sensitive to unnormalized differences (case/whitespace/float rendering) and can diverge from the matcher and Excel (which are intended to be normalization-driven). Buildgkusing the same normalized components you use elsewhere (e.g.,join_key_parts([norm_key(field(...), norm) ...])overnontiming) so offset detection matches the platform-determinism goal.
reduced_nonempty = any(norm_key(field(r["key"], k), norm) for k in nontiming)
has_offset = (timing_on and reduced_nonempty
and any(o["difftype"] == opp for o in grp.get(gk, [])))
r["rootcause"] = "Timing" if has_offset else "Scope / mapping"
submissions/reconciliation-assistant/README.md:33
- This description implies timing differences become a Needs Review state, but the implementation/methodology keeps them as
Unmatched (A)/Unmatched (B)with an annotation (and Root Cause = Timing in the workbook). Update the README wording to match the actual behavior to avoid confusing users reviewing output statuses.
3. **Similarity** — no shared key, but amount, date, and name all line up within thresholds → *Probable*, sent to Needs Review. Optional — you decide at setup whether to allow it.
4. **Grouped** — one record on one side equals several on the other (e.g. an invoice paid by three partial payments) → sent to Needs Review.
5. **Timing** — the same item posted to a *different period* on each side (same account and amount, different month) → flagged as a timing difference in Needs Review, instead of showing up as two confusing one-sided breaks. Ideal for GL, bank, and accrual reconciliations.
6. **Unmatched** — everything left, split by which side it came from.
Comment on lines
+737
to
+740
| r_first = 5 | ||
| r_last = r_first + n_lines - 1 | ||
| r_total = r_last + 1 | ||
| r_ctrl = r_total + 1 |
Comment on lines
+905
to
+916
| rng = f"A{r_first}:{L_action}{r_last}" | ||
| zebra = PatternFill(start_color=ZEBRA_BG, end_color=ZEBRA_BG, fill_type="solid") | ||
| ws.conditional_formatting.add(rng, FormulaRule(formula=["MOD(ROW(),2)=1"], fill=zebra)) | ||
| srng = f"{L_status}{r_first}:{L_status}{r_last}" | ||
| ws.conditional_formatting.add(srng, CellIsRule( | ||
| operator="equal", formula=['"Open Item"'], | ||
| fill=PatternFill(start_color=OPEN_BG, end_color=OPEN_BG, fill_type="solid"), | ||
| font=Font(color=OPEN_FONT))) | ||
| ws.conditional_formatting.add(srng, CellIsRule( | ||
| operator="equal", formula=['"Reconciled"'], | ||
| fill=PatternFill(start_color=REC_BG, end_color=REC_BG, fill_type="solid"), | ||
| font=Font(color=REC_FONT))) |
Eighth review round on reconciliation-assistant. When both sources have no data rows the union is empty, so n_lines == 0 made r_last = r_first + n_lines - 1 = 4, one less than r_first (5). That produced inverted ranges like A5:A4 in the reconciliation formulas and the conditional formatting, and the source-tab ranges inverted to $2:$1 for an empty source - either of which can yield an invalid workbook or broken formulas. Two clamps prevent any inversion: - _src_meta: last = max(n + 1, 2), so an empty source's amount/matching-key ranges are the single blank cell $2:$2 instead of $2:$1. - _write_reconciliation: r_last = max(r_first + n_lines - 1, r_first), so an empty union is the single blank row $5:$5 instead of $5:$4; row 5 stays blank and every SUM/COUNT over it evaluates to 0. The non-empty case is unchanged (max(n+1,2)==n+1 for n>=1; likewise for the row clamp). Verified: a both-empty (header-only) input now produces a valid workbook with no inverted ranges, no #REF!/#DIV/0! cells, and all five controls reading OK; the HTML dashboard renders with no CHECK; GL and WHT are unchanged (-235,500.00 / -18,874.70, all controls OK, same counts); npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
submissions/reconciliation-assistant/scripts/reconcile.py:566
- Formula-injection neutralization only checks the first character of the raw string. Spreadsheet apps may still evaluate strings with leading whitespace before
=,+,-, or@. To harden this, base the check onv.lstrip()(while still preserving the original value in the output) so inputs like' =HYPERLINK(...)'are also neutralized.
def _neutralize(v):
"""Defuse spreadsheet formula/injection: a text value that begins with = + - @ could be
executed as a formula when the workbook is opened. Since all source data is untrusted, force
such strings to literal text with a leading apostrophe. Numbers are unaffected."""
if isinstance(v, str) and v[:1] in ("=", "+", "-", "@"):
return "'" + v
return v
submissions/reconciliation-assistant/scripts/reconcile.py:1234
- This post-pass iterates every cell in every worksheet to reset fonts. For large source tabs (tens of thousands of rows), this can materially slow report generation and inflate memory usage. Prefer setting the desired font at write-time for the cells you create (or limiting the pass to the specific ranges/sheets that need it), instead of scanning the entire workbook.
for ws in wb.worksheets:
for row in ws.iter_rows():
for c in row:
if c.value is None or c.font.name == "Arial":
continue
f = c.font
c.font = Font(name=REPORT_FONT, size=f.size, bold=f.bold,
italic=f.italic, color=f.color)
submissions/reconciliation-assistant/scripts/reconcile.py:815
desc_cols.index(name)runs a linear search on every iteration, making the loop O(n^2) in the number of descriptive columns. Precompute a{name: col_idx}mapping once (similar todesc_letter) and reuse it in the loop.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
if side == "a":
Comment on lines
+1340
to
+1362
| nontiming = [k for k in a_keys if k != timing_col] | ||
| timing_on = (config["matching"].get("enableTimingDetection", True) | ||
| and timing_col is not None and len(nontiming) >= 1) | ||
| grp = {} | ||
| for r in rows: | ||
| gk = tuple(str(field(r["key"], k)) for k in nontiming) | ||
| grp.setdefault(gk, []).append(r) | ||
| for r in rows: | ||
| if r["status"] == "Reconciled": | ||
| r["rootcause"] = "—" | ||
| elif r["difftype"] == "Amount mismatch": | ||
| r["rootcause"] = "Measurement" | ||
| else: | ||
| gk = tuple(str(field(r["key"], k)) for k in nontiming) | ||
| opp = f"Missing in {la}" if r["difftype"] == f"Missing in {lb}" else f"Missing in {lb}" | ||
| # Only a row whose reduced (non-timing) key is genuinely non-blank can be a timing | ||
| # difference. norm_key collapses NaN/blank components to "" (str(NaN) would be the | ||
| # truthy "nan"), so keyless / blank-reduced rows are correctly excluded - matching the | ||
| # reconcile() and Excel guards. | ||
| reduced_nonempty = any(norm_key(field(r["key"], k), norm) for k in nontiming) | ||
| has_offset = (timing_on and reduced_nonempty | ||
| and any(o["difftype"] == opp for o in grp.get(gk, []))) | ||
| r["rootcause"] = "Timing" if has_offset else "Scope / mapping" |
Comment on lines
+845
to
+855
| # Root Cause: measurement (amount mismatch), timing (offsetting missing entry in the | ||
| # same non-period group when timing applies to this row), else scope / mapping. | ||
| if row_timing: | ||
| countifs = "" | ||
| for k in nontiming_keys: | ||
| kl = desc_letter[k] | ||
| countifs += f"${kl}$5:${kl}${r_last},${kl}{r}," | ||
| opp = f'IF({L_dtype}{r}="Missing in {lb}","Missing in {la}","Missing in {lb}")' | ||
| root = (f'=IF({L_status}{r}="Reconciled","—",' | ||
| f'IF({L_dtype}{r}="Amount mismatch","Measurement",' | ||
| f'IF(COUNTIFS({countifs}${L_dtype}$5:${L_dtype}${r_last},{opp})>0,' |
Ninth review round on reconciliation-assistant. Timing root-cause detection grouped offsetting entries by the RAW non-timing key values, ignoring the configured trim/case normalization - so the classification could diverge across platforms when non-timing key parts differ only by whitespace or case. - HTML (compute_reconciliation): the grouping key is now tuple(norm_key(field(...), norm) for each non-timing key) in both the group-building and per-row lookups, and the reduced-non-empty guard reads that same normalized tuple. - Excel (_write_reconciliation): a hidden "Reduced Key (helper)" column now holds the TRIM/LOWER-normalized reduced key (built with the same _xl_key_formula as the Matching Key), and the Root Cause timing COUNTIFS matches on that helper plus the opposite Difference Type - instead of COUNTIFS over the raw display columns. The helper sits to the right of the visible columns, is hidden, and is excluded from the totals border, the conditional formatting, and the Dashboard ranges, so nothing visible shifts. Both now match the Python matcher (which already reduces via norm_key), keeping the three platforms deterministic. Verified: with a non-timing key that differs only by case, both the HTML and the workbook now classify the pair as Timing (previously the workbook read Scope / mapping); keyless / blank-reduced rows are still never labelled Timing; GL and WHT are unchanged (-235,500.00 / -18,874.70, all controls OK, root causes 2 and 4 timing); npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
submissions/reconciliation-assistant/scripts/reconcile.py:642
- Using
df.iterrows()is significantly slower than alternatives and will materially impact runtime on large sources. Preferitertuples(index=False, name=None)(orto_numpy()/vectorized extraction) and index columns by position to reduce pandas overhead during workbook writes.
for r, (_, row) in enumerate(df.iterrows(), start=2):
for c, name in enumerate(cols, start=1):
v = row[name]
submissions/reconciliation-assistant/scripts/reconcile.py:825
- This repeatedly calls
desc_cols.index(name)inside the loop, creating unnecessary O(D²) behavior and making the column mapping logic harder to reason about. Precompute adesc_col_to_idxmapping (and optionally source-letter mappings) once, then reuse it while writing rows.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
submissions/reconciliation-assistant/scripts/reconcile.py:347
- The comment says 'Search smaller (nearest-magnitude-first)' but the code sorts by descending magnitude (
reverse=True), which is the opposite. Either update the comment to match the implemented strategy (largest-first), or change the sort order if the intended behavior is smallest-first.
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
submissions/reconciliation-assistant/scripts/reconcile.py:309
- Similarity matching is an O(|unmatched_a| × |unmatched_b|) nested loop, and within it you repeatedly parse dates with
pd.to_datetime(...). For larger datasets (or when many rows remain unmatched after Tier 1/2), this can become a bottleneck. A concrete improvement is to pre-parse the configured date columns once (vectorized) and/or pre-index B candidates by quantized amount (and optionally by date bucket/window) so each A row compares against a much smaller candidate set.
for rb in unmatched_b:
if rb["_idx"] in used_b or not str(rb["_key"]).strip():
continue
if not within_tolerance(ra["_amt"], rb["_amt"], abs_tol, pct_tol):
continue
if date_a and date_b:
da, db = ra.get(date_a), rb.get(date_b)
try:
delta_days = abs((pd.to_datetime(da) - pd.to_datetime(db)).total_seconds()) / 86400.0
except Exception:
# A date was configured but could not be parsed: the proximity rule
# cannot be satisfied, so this pair is not eligible for similarity.
continue
if delta_days > window:
continue
sim = similarity(ra["_key"], rb["_key"])
Comment on lines
+1237
to
+1245
| from openpyxl.styles import Font | ||
| for ws in wb.worksheets: | ||
| for row in ws.iter_rows(): | ||
| for c in row: | ||
| if c.value is None or c.font.name == "Arial": | ||
| continue | ||
| f = c.font | ||
| c.font = Font(name=REPORT_FONT, size=f.size, bold=f.bold, | ||
| italic=f.italic, color=f.color) |
Tenth review round on reconciliation-assistant.
Applying fonts by walking every cell of every worksheet is O(rows x cols) and
became the dominant cost (and a memory pressure point) on large reconciliations.
- The whole-workbook font post-pass is removed. Each writer now creates shared
Font objects once (Cambria body/header/matching-key, Arial pulled-text, the
Calibri narrative) and assigns them by reference as each cell is created, and
the two large writers likewise reuse shared Alignment objects instead of
constructing one per cell. openpyxl de-duplicates styles, so this keeps the
workbook identical while avoiding the second pass and the per-cell allocations.
- Per-row pandas Series allocations are eliminated: the source-tab writer iterates
df.to_dict("records") instead of df.iterrows(); the reconciliation union/timing
lookups and the dashboard account / company-period pivots index a single
to_dict("records") (with column-membership sets) instead of df.iloc[i] (which
builds a fresh Series on every access); the basis line uses df[col].tolist().
On a 20k-row synthetic reconciliation the workbook write dropped from ~40s to
~22s, and the O(rows x cols) walk is gone entirely.
Verified: GL and WHT are byte-for-byte equivalent in content (-235,500.00 /
-18,874.70, all controls OK, root causes 2 and 4 timing) and render with the same
fonts/alignment as before - a full-sheet font-parity scan shows every cell Cambria
except the Arial pulled-text cells and the five Calibri Headlines lines, exactly as
the old post-pass produced; npm test 10/10, check:submissions 88/88, build 356
pages.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
submissions/reconciliation-assistant/scripts/reconcile.py:253
- For duplicate keys (multiple B candidates with the same key), the selection only considers amount distance. This conflicts with the documented rule in
references/methodology.md(“nearest amount then nearest date”) and can pick an unintended candidate when two amounts are equally close. Consider adding a deterministic secondary tie-break using date proximity (when configured) and then_idxas a final stable tie-break.
candidates.sort(key=lambda r: abs((r["_amt"] or 0) - (ra["_amt"] or 0)))
rb = candidates[0]
submissions/reconciliation-assistant/scripts/reconcile.py:857
desc_cols.index(name)inside the per-row/per-column loop makes this O(rows * cols^2) due to repeated linear searches. Precompute a{name: col_idx}mapping once (you already computedesc_letter) and use that inside the loop to keep reconciliation-sheet generation fast for large files.
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
submissions/reconciliation-assistant/scripts/reconcile.py:347
- The comment says the search is 'smaller (nearest-magnitude-first)' but the code sorts by magnitude with
reverse=True(largest-first). Either adjust the comment to match the actual behavior or change the sort direction to align with the stated intent.
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
Comment on lines
+1044
to
+1049
| for g in group_by: | ||
| if g == group_by[0]: | ||
| # Distinct group values from the two columns directly (vectorized), instead of an | ||
| # O(rows) df.iloc[i][g] Series allocation per row. | ||
| vals_src = sorted({str(v) for v in df_a[g].tolist()} | | ||
| {str(v) for v in df_b[g].tolist()}) |
Comment on lines
+1204
to
+1208
| for side, srow in info["recon_rows"]: | ||
| rec = a_recs[srow - 2] if side == "a" else b_recs[srow - 2] | ||
| cols_set = a_cols if side == "a" else b_cols_set | ||
| comp = rec.get(group_by[0]) if group_by[0] in cols_set else None | ||
| per = rec.get(group_by[1]) if group_by[1] in cols_set else None |
Eleventh review round on reconciliation-assistant. The Dashboard basis line and the "Difference by account" / "Difference by company and period" pivots read the configured groupBy/account columns (which are A-side names) directly from df_b. When matching.keyMap maps an A key column to a differently-named B column, df_b[<A name>] raised a KeyError on the basis line and silently dropped B-origin rows from the company/period pivot (comp/per resolved to None). - A single A->B column map (keymap_ab = zip(a.keyColumns, b.keyColumns), which are positionally aligned by align_key_columns) is now built once in _write_dashboard. - The basis line, the account block, and the company/period combo loop translate every A-side group/account name to its B column name before reading b_recs, and guard on membership in the B columns. The account block now reuses this shared map instead of its own local copy. The mapped B values are exactly the values _write_reconciliation pulls into the Reconciliation descriptive columns for B-origin rows (same map, same frame, same rows), so the pivot SUMIFS/COUNTIFS criteria stay consistent with the sheet. Verified: with a config whose keyMap renames the B group columns, write_report no longer raises and a B-only (company, period) combo is captured instead of dropped, with all controls OK. The identity case (matching column names, e.g. the GL/WHT tests) is byte-for-byte unchanged (-235,500.00 / -18,874.70, all controls OK, root causes 2 and 4 timing); npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
submissions/reconciliation-assistant/scripts/reconcile.py:1294
write_reportdeclaresdf_a/df_bas optional, but unconditionally callscompute_reconciliation(df_a, df_b, config), which will fail if either frame isNone. Either makedf_a/df_brequired parameters (remove the defaultNone) or add a guard/fallback path when they aren’t provided (e.g., skip narrative / per-key computation, or raise a clear error early).
def write_report(results, config, out_path, df_a=None, df_b=None, src_name=None):
import openpyxl
la = config["sources"]["a"].get("label", "Source A")
lb = config["sources"]["b"].get("label", "Source B")
res_df = pd.DataFrame(results)
counts = res_df["status"].value_counts().to_dict() if not res_df.empty else {}
# The Dashboard headlines are built from the per-key reconciliation model (compute_reconciliation) -
# the same model that drives the Reconciliation sheet's formulas and the HTML dashboard - so the
# narrative counts can never disagree with the sheet totals. (The tiered `results`/`counts` are a
# record-level view returned for the caller's console summary, not the per-key artifact.)
perkey_rows, _, _ = compute_reconciliation(df_a, df_b, config)
submissions/reconciliation-assistant/scripts/reconcile.py:857
- Inside the per-row loop,
desc_cols.index(name)does an O(D) linear search for every descriptive column, turning the write into an avoidable O(rows * D^2) pattern. Precompute(name, col_idx)(or iterate withenumerate(desc_cols)and computecol_idxdirectly) so this inner loop stays O(rows * D).
for name in desc_cols:
col_idx = 2 + desc_cols.index(name)
if side == "a":
submissions/reconciliation-assistant/scripts/reconcile.py:347
- The comment says the search is 'smaller (nearest-magnitude-first)', but the sort is
reverse=True, which orders by largest magnitude first. Either adjust the sort order to match the comment, or update the comment to reflect the intended largest-first strategy (and why it improves pruning).
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
Comment on lines
+361
to
+365
| # One A record ↔ many B records. | ||
| for ra in unmatched_a: | ||
| if ra["_amt"] is None: | ||
| continue | ||
| combo = _find_combo(ra["_amt"], unmatched_b, used_b | grouped_b) |
Twelfth review round on reconciliation-assistant. Fixes the flagged grouped-tier bug and, via an exhaustive whole-file audit against every recurring issue-class, three latent siblings that would otherwise surface in later rounds. - Grouped matching now excludes keyless rows (all key components blank). A keyless row can no longer be a grouped target or a combo member on either side, matching the exact and similarity tiers, which already treat an empty key as non-matchable. Keyless rows stay one-sided breaks; real keyed grouped matches are unaffected. - Formula-injection hardening extended to the Dashboard. _neutralize was only applied to the source-tab data cells; the Dashboard pivot value cells (account, name, company, period), the basis line, and the header cells (source tab, Reconciliation, and Dashboard headers built from user column names) now go through it too. This also keeps the account pivot's SUMIF criteria consistent with the already-neutralized source-tab cells for values that begin with "=". - The HTML "every key appears once" control was tautological (total vs total). It now compares total to an independent unique-key count recomputed straight from the raw frames (same canonicalization as the union), mirroring the workbook's SUMPRODUCT(1/COUNTIF) control. - Performance: the reconciliation writer's per-row loop no longer calls list.index() for every descriptive column (an O(rows x cols^2) scan). The column geometry (target index and A/B source-column letters) is precomputed once and looked up by name. Verified: keyless rows never grouped and real grouped matches still work; source-derived strings (incl. "=cmd"/"=HYPERLINK" values and column names) are written as inert text, not live formulas; HTML controls all read OK via the independent count; the recon geometry hoist emits byte-identical cell references. GL and WHT are unchanged (-235,500.00 / -18,874.70, all controls OK, root causes 2 and 4 timing); npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
submissions/reconciliation-assistant/scripts/reconcile.py:568
- Formula injection can bypass this check via leading whitespace (e.g. " =SUM(...)"), since Excel may still interpret it as a formula. A safer approach is to test
v.lstrip()for the dangerous prefix (while still preserving the original string content once escaped), so values with leading spaces are also neutralized.
def _neutralize(v):
"""Defuse spreadsheet formula/injection: a text value that begins with = + - @ could be
executed as a formula when the workbook is opened. Since all source data is untrusted, force
such strings to literal text with a leading apostrophe. Numbers are unaffected."""
if isinstance(v, str) and v[:1] in ("=", "+", "-", "@"):
return "'" + v
return v
submissions/reconciliation-assistant/scripts/reconcile.py:349
- The comment says the search is 'nearest-magnitude-first' / smaller first, but the implementation sorts by absolute amount in descending order (
reverse=True), which is largest-first. Either update the comment to match the actual heuristic, or change the sort order if the intended behavior is to try smaller members first.
# Search smaller (nearest-magnitude-first) combinations first, with an attempt cap
# so a pathological pool can't blow up the run.
avail.sort(key=lambda r: abs(r["_amt"]), reverse=True)
submissions/reconciliation-assistant/scripts/reconcile.py:1347
compute_reconciliation()usesoutput.groupByas-is, while the Excel Dashboard path filters group-by columns to those present in A's key columns (see_write_dashboard()), to avoid missing/empty dimensions and keep pivots consistent. To prevent HTML/workbook divergence, apply the same filtering incompute_reconciliation()(and/or validate the config) so the per-key model fields (company,period) are populated consistently across platforms.
out = config.get("output", {})
acct_col = out.get("accountColumn")
name_col = out.get("accountNameColumn")
group_by = out.get("groupBy", [])
submissions/reconciliation-assistant/scripts/reconcile.py:1838
- This treats negative numeric strings (e.g. "-1") as a valid sheet index and passes them through to
pandas.read_excel(sheet_name=...). Pandas' sheet indices are non-negative; negative indices will likely error and are confusing as a CLI affordance. Consider rejecting negative indices (or only accepting digits without a leading '-') and emitting a clear error message.
def _parse_sheet(s):
if isinstance(s, str) and s.strip().lstrip("-").isdigit():
return int(s.strip())
return s
submissions/reconciliation-assistant/scripts/reconcile.py:1878
- The workbook reconciliation key formula on the Reconciliation sheet intentionally excludes the amount column from descriptive columns (
desc_cols), but the source-tab Matching Key helper includes every configuredkeyColumns. If a user accidentally includesamountColumninkeyColumns, Python matching and the source-tab keys will include it, while the Reconciliation sheet key rebuild may omit it—leading to inconsistent SUMIF/COUNTIF behavior. Consider adding an explicit config validation here: fail fast ifamountColumnappears inkeyColumnsfor either source, with an actionable message.
# Confirm configured columns exist before matching.
for side, df in (("a", df_a), ("b", df_b)):
s = config["sources"][side]
needed = list(s["keyColumns"]) + [s["amountColumn"]]
missing = [c for c in needed if c not in df.columns]
if missing:
sys.exit(f"Source '{s['label']}' is missing configured column(s): {missing}. "
f"Available: {list(df.columns)}")
Comment on lines
+455
to
+457
| closed = abs(left - explained) <= max(abs_tol, 0.01) | ||
| return {"total_a": total_a, "total_b": total_b, "net_difference": left, | ||
| "explained": explained, "residual": left - explained, "tied_out": closed} |
Thirteenth review round on reconciliation-assistant. tie_out() accepted the reconciliation identity when the residual was within max(abs_tol, 0.01). In exact mode (amountMatch "exact", abs_tol 0) that 0.01 floor masked a genuine one-cent residual - a real break - as "tied out". Since all amounts are quantized to cents, the identity is now evaluated at cent precision: abs(round(left - explained, 2)) <= abs_tol. Rounding removes binary floating-point accumulation noise (which would otherwise make a clean sum look non-zero) without hiding a real one-cent difference, and a configured absolute tolerance is still honored. The raw (unrounded) residual is still returned for display. Verified: exact mode now reports a genuine 1-cent residual as NOT tied while floating-point noise still ties; tolerance mode still accepts within-tolerance residuals and rejects larger ones; GL and WHT still tie out (net -235,500.00 / -18,874.70); npm test 10/10, check:submissions 88/88, build 356 pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Suppressed comments (2)
submissions/reconciliation-assistant/scripts/reconcile.py:574
_neutralize()only checks the first character. In Excel, values with leading whitespace (e.g., " =SUM(...)" or "\t@cmd") can still be interpreted as formulas depending on how the application trims/parses input. Recommendation (moderate): detect the first non-whitespace character (e.g., vialstrip()for the check) while preserving the original value (prepend'to the original string) when neutralization is required.
def _neutralize(v):
"""Defuse spreadsheet formula/injection: a text value that begins with = + - @ could be
executed as a formula when the workbook is opened. Since all source data is untrusted, force
such strings to literal text with a leading apostrophe. Numbers are unaffected."""
if isinstance(v, str) and v[:1] in ("=", "+", "-", "@"):
return "'" + v
return v
submissions/reconciliation-assistant/scripts/reconcile.py:1841
_parse_sheet()treats negative numeric strings (e.g., "-1") as a sheet index due tolstrip(\"-\"). The comment describes "digit-only selector" and gives examples like0, which implies non-negative indices only. Recommendation (nit): restrict this to non-negative integers (useisdigit()on the stripped string without allowing a leading '-') or clarify in help text/comments that negative indices are supported and what they mean withpandas.read_excel().
def _parse_sheet(s):
if isinstance(s, str) and s.strip().lstrip("-").isdigit():
return int(s.strip())
return s
Comment on lines
+832
to
+835
| mk_a = f"'{sa}'!${meta_a['mk_letter']}$2:${meta_a['mk_letter']}${meta_a['last']}" | ||
| amt_a_rng = f"'{sa}'!${meta_a['amt_letter']}$2:${meta_a['amt_letter']}${meta_a['last']}" | ||
| mk_b = f"'{sb}'!${meta_b['mk_letter']}$2:${meta_b['mk_letter']}${meta_b['last']}" | ||
| amt_b_rng = f"'{sb}'!${meta_b['amt_letter']}$2:${meta_b['amt_letter']}${meta_b['last']}" |
Comment on lines
+897
to
+902
| c_st = ws.cell(row=r, column=7 + D, value=f'=IF({L_dtype}{r}="None","Reconciled","Open Item")') | ||
| c_st.alignment = a_left; c_st.font = f_body | ||
| c_dt = ws.cell(row=r, column=8 + D, | ||
| value=(f'=IF({L_lines_a}{r}=0,"Missing in {la}",' | ||
| f'IF({L_lines_b}{r}=0,"Missing in {lb}",' | ||
| f'IF(ROUND({L_diff}{r},2)=0,"None","Amount mismatch")))')) |
Comment on lines
+281
to
+285
| if m.get("enableSimilarityMatching", True): | ||
| date_a = src["a"].get("dateColumn") | ||
| date_b = src["b"].get("dateColumn") | ||
| window = m.get("dateWindowDays", 3) | ||
| sim_thr = m.get("similarityThreshold", 0.9) |
Comment on lines
+299
to
+309
| if date_a and date_b: | ||
| da, db = ra.get(date_a), rb.get(date_b) | ||
| try: | ||
| delta_days = abs((pd.to_datetime(da) - pd.to_datetime(db)).total_seconds()) / 86400.0 | ||
| except Exception: | ||
| # A date was configured but could not be parsed: the proximity rule | ||
| # cannot be satisfied, so this pair is not eligible for similarity. | ||
| continue | ||
| if delta_days > window: | ||
| continue | ||
| sim = similarity(ra["_key"], rb["_key"]) |
…s for similarity tier Reconciliation formulas interpolated source-tab sheet names and source labels directly into Excel formula strings. A sheet name containing an apostrophe (e.g. a label like O'Brien) broke every 'name'!range reference, and a label containing a double-quote broke the "Missing in <label>" string literals (and allowed formula injection via the label). Add two helpers and route all source references through them: - _xl_sheet_ref(sheet, a1): doubles embedded apostrophes and wraps as 'name'!a1, applied to every source-tab range/cell reference in _write_reconciliation and the dashboard control panel. - _xl_str_literal(s): doubles embedded double-quotes and wraps in "...", applied to the "Missing in <label>" literals in the Difference Type formula and the Root Cause COUNTIFS. Precomputed once before the row loop. The runtime value stays the plain "Missing in <label>", matching the plain dashboard pivot label written as a cell value, so COUNTIF/COUNTIFS still bind. Also gate the similarity tier on both sources having a dateColumn. Similarity pairs on amount + date proximity + name; without dates only amount and name remain, which fabricates Probable pairs on common round amounts. The tier is now skipped entirely when either date column is absent, and the redundant inner per-pair date guard is removed. methodology.md and SKILL.md updated to state dates are required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (7)
submissions/reconciliation-assistant/scripts/reconcile.py:577
_neutralizeonly checks the very first character. Excel formula injection can still occur when a value starts with whitespace followed by=,+,-, or@(e.g.' =1+1'). Consider checking the first non-whitespace character (e.g., usinglstrip()for the check) while preserving the original value when prefixing with'.
def _neutralize(v):
"""Defuse spreadsheet formula/injection: a text value that begins with = + - @ could be
executed as a formula when the workbook is opened. Since all source data is untrusted, force
such strings to literal text with a leading apostrophe. Numbers are unaffected."""
if isinstance(v, str) and v[:1] in ("=", "+", "-", "@"):
return "'" + v
return v
submissions/reconciliation-assistant/scripts/reconcile.py:1319
write_reportdeclaresdf_a/df_bas optional (Nonedefault) but uses them unconditionally (e.g.,compute_reconciliation(df_a, df_b, ...)and_src_meta(df_a, ...)). This makes the function unsafe to call unless callers always pass both dataframes. Either (a) makedf_aanddf_brequired parameters (preferred), or (b) add an explicit early validation with a clear error message when either isNone.
def write_report(results, config, out_path, df_a=None, df_b=None, src_name=None):
submissions/reconciliation-assistant/scripts/reconcile.py:1323
write_reportdeclaresdf_a/df_bas optional (Nonedefault) but uses them unconditionally (e.g.,compute_reconciliation(df_a, df_b, ...)and_src_meta(df_a, ...)). This makes the function unsafe to call unless callers always pass both dataframes. Either (a) makedf_aanddf_brequired parameters (preferred), or (b) add an explicit early validation with a clear error message when either isNone.
res_df = pd.DataFrame(results)
submissions/reconciliation-assistant/scripts/reconcile.py:1333
write_reportdeclaresdf_a/df_bas optional (Nonedefault) but uses them unconditionally (e.g.,compute_reconciliation(df_a, df_b, ...)and_src_meta(df_a, ...)). This makes the function unsafe to call unless callers always pass both dataframes. Either (a) makedf_aanddf_brequired parameters (preferred), or (b) add an explicit early validation with a clear error message when either isNone.
perkey_rows, _, _ = compute_reconciliation(df_a, df_b, config)
narrative = _build_narrative_perkey(perkey_rows, config)
meta_a = _src_meta(df_a, config["sources"]["a"], config)
meta_b = _src_meta(df_b, config["sources"]["b"], config)
submissions/reconciliation-assistant/scripts/reconcile.py:1529
df_a.to_dict('records')/df_b.to_dict('records')are computed multiple times inbuild_html_dashboard. For large reconciliations this can be a noticeable overhead and duplicates memory. Consider materializinga_recs/b_recsonce at the start ofbuild_html_dashboard(when frames are provided) and reusing them for both the independent totals and the independent key-count control.
if df_a is not None and df_b is not None:
ind_a = round(sum(apply_sign(normalize_amount(rec.get(amt_a_col), nrm), sgn_a) or 0.0
for rec in df_a.to_dict("records")), 2)
ind_b = round(sum(apply_sign(normalize_amount(rec.get(amt_b_col), nrm), sgn_b) or 0.0
for rec in df_b.to_dict("records")), 2)
submissions/reconciliation-assistant/scripts/reconcile.py:1546
df_a.to_dict('records')/df_b.to_dict('records')are computed multiple times inbuild_html_dashboard. For large reconciliations this can be a noticeable overhead and duplicates memory. Consider materializinga_recs/b_recsonce at the start ofbuild_html_dashboard(when frames are provided) and reusing them for both the independent totals and the independent key-count control.
a_ky = {_canon(rec, a_keycols, "A " + la, i) for i, rec in enumerate(df_a.to_dict("records"))}
b_extra = {k for j, rec in enumerate(df_b.to_dict("records"))
if (k := _canon(rec, b_keycols, "B " + lb, j)) not in a_ky}
submissions/reconciliation-assistant/scripts/reconcile.py:1599
- The control status badge always uses
class=\"ok\"even when the status isCHECK, so failed controls will be styled the same as passing controls. Use a distinct class for the failing state (e.g.check) and add corresponding CSS, or set the class dynamically based onok.
ctrl_html = "".join(
f'<tr><td>{e(lbl)}</td><td class="num">{res}</td><td class="num">{exp}</td>'
f'<td><span class="ok">{"OK" if ok else "CHECK"}</span></td></tr>'
for lbl, res, exp, ok in ctrls)
…x HTML control badge
Address four review observations on the reconciliation script:
- _neutralize now inspects the first non-whitespace character (via
lstrip()) so a value like " =1+1" - leading whitespace followed by a
formula character, which Excel still evaluates - is forced to literal
text. The original value (including its whitespace) is preserved after
the apostrophe.
- write_report now takes df_a and df_b as required parameters (they were
already used unconditionally to build the per-key model, source
metadata, and tabs) with an explicit ValueError guard, so it can no
longer be called in a half-initialized state.
- build_html_dashboard materializes df_a/df_b to_dict("records") once
into a_recs/b_recs and reuses them for both the independent-totals and
the independent-key-count controls, avoiding a duplicate O(rows*cols)
pass on large reconciliations.
- The HTML control-panel status badge now uses class "check" (with a new
red .check CSS rule) when a control reads CHECK, so a failing control
is visually distinct from a passing one instead of always styled OK.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
submissions/reconciliation-assistant/README.md:34
- This describes timing differences as "flagged ... in Needs Review", but the reference implementation and methodology keep them as
Unmatched (A)/Unmatched (B)with an annotation (and workbook Root Cause = Timing). To avoid confusing users, align README wording with the actual behavior (e.g., "annotated as a possible timing difference" while remaining unmatched/open).
3. **Similarity** — no shared key, but amount, date, and name all line up within thresholds → *Probable*, sent to Needs Review. Optional — you decide at setup whether to allow it.
4. **Grouped** — one record on one side equals several on the other (e.g. an invoice paid by three partial payments) → sent to Needs Review.
5. **Timing** — the same item posted to a *different period* on each side (same account and amount, different month) → flagged as a timing difference in Needs Review, instead of showing up as two confusing one-sided breaks. Ideal for GL, bank, and accrual reconciliations.
6. **Unmatched** — everything left, split by which side it came from.
| amt = float(s) | ||
| except ValueError: | ||
| return None | ||
| return round(-amt if negative else amt, 2) |
normalize_amount treats an accounting value wrapped in parentheses as negative, then returned -amt. When the parenthesized value ALSO carried a minus sign - e.g. "(-50.00)" - the parsed float was already -50, so -amt produced +50, silently flipping the sign and masking a real break. Use -abs(amt) when parentheses indicate negative: the parentheses are authoritative for the sign and the magnitude comes from the parsed number, so "(-50.00)", "(50.00)" and "$(1,250.00)" all resolve to the correct negative, while plain "-50.00" and "50.00" are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reconciliation Assistant
A domain-agnostic skill that reconciles two datasets meant to describe the same records — GL vs sub-ledger, bank vs ledger, invoices vs payments, a register vs an external report, system-of-record vs export — into a clear, review-ready account of what matches, what differs, and what is unmatched on each side, with a mandatory tie-out.
What it does
Platforms
Cowork,Copilot Studio,Scout. The reference implementation (scripts/reconcile.py) is pure, cross-OS Python (pandas + openpyxl); on Copilot Studio the same method is applied analytically over tables in context. Column labels are derived from the user's own file/tab names, and the account/company-period headers derive from the configured columns, so the output reads correctly in any domain.Files
SKILL.md(agent SOP),README.md(human overview),references/methodology.md+references/platform-notes.md,assets/config.example.json,scripts/reconcile.py.Testing
Validated with
npm run check:submissions(passes),npm test, andnpm run build. The reference script was exercised end-to-end on multiple independent datasets (GL-vs-internal and an AP-invoices-vs-WHT-register reconciliation across two tabs of one workbook); the emitted workbook's live formulas were evaluated to confirm the tie-out control proves to nil and the classifications (amount mismatch, duplicate detection via line counts, missing-in-source, and timing differences) are correct.Co-authored with GitHub Copilot.