[mvanhorn]

What changed and why

tools/convert_salary_excel.py detects the IDA header Firmanavn og by (#615). That column stores the company and the city in one cell, Ørsted A/S,Fredericia, with no space after the comma. Convert copied the whole cell into company with city: "". salary_lookup.py --city only reads entry["city"], so the documented command in tools/README_SALARY_TOOL.md misses, and /apply (which adds --city whenever a posting names a city) silently loses the salary benchmark:

python3 salary_lookup.py "Ørsted" --city "Fredericia"

Fix: when the company header also names a city (by / city / ...) and there is no separate city column, the cell is split on the last comma (rsplit(",", 1)) and both parts are stripped. Splitting on the last comma keeps a company name that itself contains ", " intact: Rambøll Danmark A/S, Odense,Odense C becomes company Rambøll Danmark A/S, Odense, city Odense C. A plain Company header still keeps commas in the name (Acme, Inc.), and a combined cell with no comma stays company-only.

The docstrings, the CHANGELOG entry (under Unreleased), tools/README_SALARY_TOOL.md and the salary benchmarking section of README.md describe the split.

Not in this PR: the optional --validate duplicate check on company plus city. It lives in salary_lookup.py; happy to send it as a follow-up.

Failing case / reproduction (for fixes)

Workbook with header Firmanavn og by | Antal | Indeks and row Ørsted A/S,Fredericia | 200 | 105.2. Run the documented convert CLI, then the documented lookup CLI.

Master convert output:

{ "company": "Ørsted A/S,Fredericia", "city": "" }

Master lookup:

exit 1
No results found for 'Ørsted'
  (filtered by city: Fredericia)

After convert:

{ "company": "Ørsted A/S", "city": "Fredericia" }

After lookup: exit 0, Ørsted A/S / Location: Fredericia / index 105.2.

Verification

• tests/test_salary_combined_city_cli.py runs convert then salary_lookup.py "Ørsted" --city "Fredericia" on a real .xlsx with the unspaced IDA cell shape.
• Unit tests in tests/test_convert_salary_excel.py: the IDA header splits Example A/S,Aarhus; Company and City splits a multi-word city (Ørsted A/S,Kongens Lyngby); a company containing ", " (Rambøll Danmark A/S, Odense,Odense C) splits on the last comma only; Company + Acme, Inc. does not split; a combined cell with no comma stays company-only.
• After merging current master: python3 -m unittest discover -s tests -t .: 806 tests, 7 skipped, OK.
• python3 tools/lint_skills.py, python3 tools/check_framework_version.py (against master), python3 tools/security_guards.py: OK.
• No CLI under .agents/skills/ is touched, so bun test / bun run typecheck do not apply.

[MadsLorentzen]

Thanks. The bug is real, and it shows up in the workflow: /apply adds --city whenever the posting names a city. On master, none of the 1,706 real rows that carry a city can be found that way, so the salary benchmark silently drops out.

But the real IDA cell has no space after the comma. It reads Ørsted A/S,Fredericia, not Ørsted A/S, Fredericia. 1,677 of the 1,708 cells with a comma look like that, and some company names contain ", " themselves (Rambøll Danmark A/S, Odense,Odense C). Splitting on the last ", " finds only 93 of the 1,706 rows with --city, and the README example still misses. That's partly on us: the #615 fixture you started from used the spaced shape.

Ask:

• Split on the last comma (rsplit(",", 1)) and strip both parts. With that change --city finds 1,706 of 1,706, no lookup without --city gets worse, and all 45 of your tests still pass.
• Use the real cell shape in the fixtures, and add a company name that contains a comma.
• Update the wording in the PR body, CHANGELOG, docstring and README.

Optional: --validate now warns about more duplicate names (802 to 907), because several sites share a company name. Checking duplicates on company plus city brings it back to 803. Merge on push.

[mvanhorn]

Thanks for checking against the real workbook. The cell is now split on the last comma (rsplit(",", 1)) with both parts stripped. The fixtures use the unspaced shape ("Ørsted A/S,Fredericia"), and a new test covers "Rambøll Danmark A/S, Odense,Odense C", which comes out as company "Rambøll Danmark A/S, Odense" with city "Odense C". The PR body, CHANGELOG, docstrings, tools/README_SALARY_TOOL.md and the README salary section are updated, and the branch is merged with current master. Locally the full unittest suite passes (806 tests, 7 skipped), as do lint_skills, check_framework_version and security_guards. I left the optional company-plus-city duplicate check in --validate out of this PR. Happy to send it as a follow-up.

[cubic-dev-ai]

6 issues found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/test_convert_salary_excel.py">

<violation number="1" location="tests/test_convert_salary_excel.py:176">
P2: This fixture change drops the only case that checks trimming. Restore the space after the comma so the test covers the stripping that the PR describes; the expected values still hold.</violation>
</file>

<file name="tools/README_SALARY_TOOL.md">

<violation number="1" location="tools/README_SALARY_TOOL.md:82">
P3: The new bullet says the cell is always split on the last comma. It is not: a cell with no comma, or with an empty part after stripping, is kept whole with an empty city. Add one sentence so users know those cells stay unsplit.</violation>
</file>

<file name="README.md">

<violation number="1" location="README.md:334">
P3: This overstates the converter: a company name containing ", " stays intact only when the cell also ends with a city. In a combined-header workbook, a cell like `Foo, Bar A/S` with no city is split into company `Foo` and city `Bar A/S`. Qualify the sentence to say so.</violation>
</file>

<file name="tools/convert_salary_excel.py">

<violation number="1" location="tools/convert_salary_excel.py:360">
P2: In a combined name-and-city column, a company cell with no city but a comma in its legal name is split and gets a fake city. `"Acme, Inc."` becomes company `"Acme"` with city `"Inc."`. The plain `Company` header avoids this, and a test pins that behavior, but this path has no equivalent guard or test.</violation>
</file>

<file name="CHANGELOG.md">

<violation number="1" location="CHANGELOG.md:17">
P3: `1.7.4` omits the release date while every earlier versioned section includes one, so the changelog no longer records when this release shipped. Add the actual release date.</violation>

<violation number="2" location="CHANGELOG.md:31">
P1: This entry documents release automation and a changelog-freeze guard that are not implemented in `HEAD`: the referenced workflow, Commitizen config, and release test are absent, while the existing structure test accepts a missing `[Unreleased]` section and never checks diffs. Add the missing automation/guard or remove these claims; otherwise releases will not be tagged and future changelog edits will not be blocked as described.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic</sub>

[cubic-dev-ai]

P1: This entry documents release automation and a changelog-freeze guard that are not implemented in HEAD: the referenced workflow, Commitizen config, and release test are absent, while the existing structure test accepts a missing [Unreleased] section and never checks diffs. Add the missing automation/guard or remove these claims; otherwise releases will not be tagged and future changelog edits will not be blocked as described.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At CHANGELOG.md, line 31:

<comment>This entry documents release automation and a changelog-freeze guard that are not implemented in `HEAD`: the referenced workflow, Commitizen config, and release test are absent, while the existing structure test accepts a missing `[Unreleased]` section and never checks diffs. Add the missing automation/guard or remove these claims; otherwise releases will not be tagged and future changelog edits will not be blocked as described.</comment>

<file context>
@@ -25,6 +28,16 @@ per-file diff commands.
 
 ### Changed
 
+- **Releases are weekly and automatic, and this changelog is frozen** (`.github/workflows/release.yml`,
+  `.cz.toml`, `tests/test_release_workflow.py`, `tests/test_changelog_structure.py`) - every
+  PR edited `[Unreleased]` by hand, so every PR after the first conflicted on this file, and only one
</file context>

[cubic-dev-ai]

P2: This fixture change drops the only case that checks trimming. Restore the space after the comma so the test covers the stripping that the PR describes; the expected values still hold.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/test_convert_salary_excel.py, line 176:

<comment>This fixture change drops the only case that checks trimming. Restore the space after the comma so the test covers the stripping that the PR describes; the expected values still hold.</comment>

<file context>
@@ -173,13 +173,60 @@ def test_parse_sheet_detects_danish_compound_company_header(self):
         ws = FakeWorksheet([
             ("Firmanavn og by", "Antal", "Indeks"),
-            ("Example A/S, Aarhus", 12, 105.0),
+            ("Example A/S,Aarhus", 12, 105.0),
         ])
         companies = parse_sheet(ws)
</file context>

Suggested change

	("Example A/S,Aarhus", 12, 105.0),
	("Example A/S, Aarhus", 12, 105.0),

[cubic-dev-ai]

P2: In a combined name-and-city column, a company cell with no city but a comma in its legal name is split and gets a fake city. "Acme, Inc." becomes company "Acme" with city "Inc.". The plain Company header avoids this, and a test pins that behavior, but this path has no equivalent guard or test.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tools/convert_salary_excel.py, line 360:

<comment>In a combined name-and-city column, a company cell with no city but a comma in its legal name is split and gets a fake city. `"Acme, Inc."` becomes company `"Acme"` with city `"Inc."`. The plain `Company` header avoids this, and a test pins that behavior, but this path has no equivalent guard or test.</comment>

<file context>
@@ -327,6 +357,8 @@ def metadata_column(patterns, skip=None):
             city_name = str(row[city_col]).strip()
         else:
             city_name = ""
+        if combined_name_and_city and not city_name:
+            company_name, city_name = split_combined_company_city(company_name)
 
</file context>

[cubic-dev-ai]

P3: The new bullet says the cell is always split on the last comma. It is not: a cell with no comma, or with an empty part after stripping, is kept whole with an empty city. Add one sentence so users know those cells stay unsplit.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tools/README_SALARY_TOOL.md, line 82:

<comment>The new bullet says the cell is always split on the last comma. It is not: a cell with no comma, or with an empty part after stripping, is kept whole with an empty city. Add one sentence so users know those cells stay unsplit.</comment>

<file context>
@@ -79,6 +79,7 @@ On Windows, use `py` if that is how Python is exposed on your PATH. If your syst
 
 The converter auto-detects the Excel layout:
 - Looks for a "Company"/"Firma" column and an optional "City"/"By" column
+- A combined header such as "Firmanavn og by" or "Company and City" (no separate city column) is split on the last comma, with both parts stripped, so `salary_lookup.py --city` can filter. Cells look like "Ørsted A/S,Fredericia"; a company name may itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C")
 - Treats remaining columns as salary data (auto-pairs count/index columns)
 
</file context>

Suggested change

- A combined header such as "Firmanavn og by" or "Company and City" (no separate city column) is split on the last comma, with both parts stripped, so `salary_lookup.py --city` can filter. Cells look like "Ørsted A/S,Fredericia"; a company name may itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C")

- A combined header such as "Firmanavn og by" or "Company and City" (no separate city column) is split on the last comma, with both parts stripped, so `salary_lookup.py --city` can filter. Cells look like "Ørsted A/S,Fredericia"; a company name may itself contain ", " ("Rambøll Danmark A/S, Odense,Odense C"). Cells without a comma, or with an empty part after stripping, are kept whole with no city.

[cubic-dev-ai]

P3: This overstates the converter: a company name containing ", " stays intact only when the cell also ends with a city. In a combined-header workbook, a cell like Foo, Bar A/S with no city is split into company Foo and city Bar A/S. Qualify the sentence to say so.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At README.md, line 334:

<comment>This overstates the converter: a company name containing ", " stays intact only when the cell also ends with a city. In a combined-header workbook, a cell like `Foo, Bar A/S` with no city is split into company `Foo` and city `Bar A/S`. Qualify the sentence to say so.</comment>

<file context>
@@ -331,6 +331,8 @@ Market-specific *data sources* (a national salary database, local award-rate tab
 
 The salary tool works with any salary data you provide (union statistics, Glassdoor exports, personal research, etc.). See `tools/README_SALARY_TOOL.md` for the expected format and setup. If you don't have salary data, the salary step is simply skipped.
 
+`tools/convert_salary_excel.py` turns an Excel export into that format. When the company column also holds the city, as in the IDA workbook's "Firmanavn og by" column (`Ørsted A/S,Fredericia`), the converter splits each cell on its last comma and strips both parts, so `salary_lookup.py --city` can filter. A company name that itself contains ", " stays intact.
+
 ### Starting over
</file context>

Suggested change

`tools/convert_salary_excel.py` turns an Excel export into that format. When the company column also holds the city, as in the IDA workbook's "Firmanavn og by" column (`Ørsted A/S,Fredericia`), the converter splits each cell on its last comma and strips both parts, so `salary_lookup.py --city` can filter. A company name that itself contains ", " stays intact.

`tools/convert_salary_excel.py` turns an Excel export into that format. When the company column also holds the city, as in the IDA workbook's "Firmanavn og by" column (`Ørsted A/S,Fredericia`), the converter splits each cell on its last comma and strips both parts, so `salary_lookup.py --city` can filter. A company name containing ", " stays intact only when the cell also ends with a city; a comma-containing name without a city is split at that comma.

[cubic-dev-ai]

P3: 1.7.4 omits the release date while every earlier versioned section includes one, so the changelog no longer records when this release shipped. Add the actual release date.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At CHANGELOG.md, line 17:

<comment>`1.7.4` omits the release date while every earlier versioned section includes one, so the changelog no longer records when this release shipped. Add the actual release date.</comment>

<file context>
@@ -1,17 +1,20 @@
 per-file diff commands.
 
-## [Unreleased]
+## [1.7.4]
 
 ### Added
</file context>

Suggested change

	## [1.7.4]
	## [1.7.4] - 2026-11-09

[MadsLorentzen]

All three Oct-19 asks landed (last-comma split, real unspaced fixtures, a comma-bearing company name). I dropped the CHANGELOG hunk on your branch since the file is frozen by #800; release notes come from the PR title. Verified on Windows.

[mvanhorn]

Thanks for merging this @MadsLorentzen. Splitting the combined Firmanavn og by cell into company and city should let salary_lookup.py --city find those IDA rows again.