Skip to content

Refuse writes for employees who work at more than one location - #187

Merged
tmfahey merged 5 commits into
mainfrom
multi-location-guard
Nov 5, 2026
Merged

tmfahey merged 5 commits into
mainfrom
multi-location-guard

Conversation

@tmfahey

@tmfahey tmfahey commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • An employee who works at more than one location has one active work address per location. Hours sent through payroll update or timesheet create can't say which location they were worked at, and the API puts payroll hours on the employee's primary location without complaint. employee update picks which active address to overwrite, so with two of them it would be guessing.
  • Those commands now check before sending. If an employee works at more than one location, nothing is saved, and the command refuses with multi_location_not_supported (exit 8) and names them in error.employee_uuids. For payroll update, clearing those employees' hours cells lets the rest of the file through, so bonus and tips still land.
  • payroll update only looks up employees whose rows have hours, deduped and at most 4 at a time. A 429 is retried using Retry-After. If a lookup fails, the employees it did find are still named; if it found no one, the write fails and says who couldn't be checked. A 404 is left for the update to report. --dry-run runs the same check when credentials are available.
  • employee update-work-address isn't refused. It changes only the address it names, so there's nothing to guess.
  • The check only sees addresses active today, so a second location that has ended or hasn't started yet goes unnoticed. AGENTS.md says so.
  • --token-stdin now keeps the piped token for the life of the process, since payroll update resolves auth more than once. A test reset clears it between tests.
  • payroll update now reports a stale version as version_conflict (exit 8) with payroll prepare as the fix, like the other versioned writes, instead of a generic api_client_error.

Linked issue

None

Test plan

  • bun run test:all passes locally
  • Manual run of touched commands works against sandbox. I don't have a sandbox company with a multi-location employee, so the checks are covered by network tests only.
  • --agent and --human output verified where touched

DCO

  • Every commit is signed off (git commit -s) per the DCO

Signed-off-by: tmfahey <taylor.fahey@gusto.com>
…efusal

Signed-off-by: tmfahey <taylor.fahey@gusto.com>
…inputs on refusal

Signed-off-by: tmfahey <taylor.fahey@gusto.com>
…to the review

Signed-off-by: tmfahey <taylor.fahey@gusto.com>
@tmfahey
tmfahey marked this pull request as ready for review November 3, 2026 22:23
@tmfahey
tmfahey requested review from a team and ashieh as code owners November 3, 2026 22:23

@jeff-gusto jeff-gusto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for three issues in the payroll update multi-location path (see inline). The employee-command prechecks look good. I have a few smaller cleanup notes too and will send them separately.

Comment thread src/commands/payroll.ts Resolved
Comment thread src/commands/payroll.ts Resolved
Comment thread src/commands/payroll.ts Resolved
@jeff-gusto

Copy link
Copy Markdown
Contributor

A few smaller cleanup notes from the review. None of these block the PR.

  1. update-work-address reads the same address twice (src/lib/multi-location.ts, workAddressMultiLocationCheck). The precheck GETs /v1/work_addresses/{uuid}, then putResourceWithVersion GETs that same path again to read the version. That's three reads before the PUT. It also leaves a window where the version can change after the multi-location decision, and the precheck still adds a read when --record-version is passed, which is meant to skip it. Could the precheck's read also supply the version?

  2. Duplicated detection logic (src/commands/employee.ts, employeeUpdateHandler). This repeats the fetch, the non-array check and the multi-location test that employeeIsMultiLocation already does. The /v1/employees/{uuid}/work_addresses path is now built in three places. If the detection rule changes (say, to count future-dated addresses), employee update and update-work-address could disagree. Can employeeUpdateHandler reuse the helper and share one path builder?

  3. The stdin token cache can't be reset (src/lib/stdin.ts). I get why it's cached: auth can be resolved twice and the pipe is empty after the first read. The multi-location refusal in payroll update depends on that. But the cache lives at module level, and Bun's test runner shares one module instance across tests. So the first test that reads real stdin (a TTY or empty stdin gives null) decides the token for every later test that doesn't inject input. A small exported reset for tests would close that off.

  4. The two --dry-runs disagree. For the same multi-location employee, employee update --dry-run returns multi_location_not_supported (it reads the addresses before it can build the preview), but update-work-address --dry-run skips the precheck and returns a successful preview. An agent can preview the write, get operator approval, and only hit the refusal on --confirm. Either run the precheck on dry-run when auth is available, or say in the help text that dry-run doesn't check for multiple locations.

Signed-off-by: tmfahey <taylor.fahey@gusto.com>
@tmfahey

tmfahey commented Nov 4, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, all four are in 7efd738:

  1. I dropped the update-work-address refusal rather than merging the reads. I know you said the employee prechecks looked good, so flagging it: that PUT names one address and only changes that one, so there's no location to guess. The precheck, the extra read and the version window go with it.
  2. employee update uses the shared fetch, and the work-address path is built in one place.
  3. Added resetStdinTokenCache, and the test preload calls it before each test.
  4. payroll update and timesheet create run the check on --dry-run when credentials are available. After 1, update-work-address has no check to disagree about.

On counting future-dated addresses: the response has no end date or primary flag, so a planned move to a new location looks the same as a second location starting. Counting them would refuse ordinary relocations, so the check stays on addresses active today, and AGENTS.md calls out the gap.

@tmfahey
tmfahey requested a review from jeff-gusto November 4, 2026 17:50

@jeff-gusto jeff-gusto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Taylor, 7efd738 covers everything. Checking before the write is the right fix, and agreed on dropping the update-work-address refusal since that PUT names its address. LGTM.

@tmfahey
tmfahey merged commit c5253b3 into main Nov 5, 2026
14 checks passed
@tmfahey
tmfahey deleted the multi-location-guard branch November 5, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants