From d7500793e2b5fa1dc611152dacf72da974c6dd63 Mon Sep 17 00:00:00 2001 From: Lucy Date: Fri, 28 Aug 2026 16:16:51 +0000 Subject: [PATCH] fix(hire): make departmentId optional, because its description promised a default The field was REQUIRED under a description opening 'DEFAULT: the department YOU head'. An agent read that, reasoned correctly that it should omit the field, met a schema that would not allow it, and improvised the most salient name in context -- the company's. It obeyed the instrument over the claim, which is the right thing for it to do. The field is optional now and the default is resolved in execute using the helper that already existed: the department the caller heads, or failing that the one they sit in. That is word-for-word what the description promised, so the promise was always implementable and simply was not implemented. The worked example now shows the omitted form first. An example that keeps passing the field re-teaches the habit the description is trying to correct. The sweep asked the general question of every parameter in the file -- does the schema permit what the prose promises -- across DEFAULT, omit, omitted, optional and leave empty. Exactly one leaf field disagreed: this one. The only other hit says 'never omit this field' and is required, so its prose and schema agree. No mechanical guard was added: an instrument for a class of one is furniture. My first sweep reported sixteen mismatches. All but one were container objects matching on a nested field's prose -- the detector measured a property adjacent to the question. Refining it to leaf fields gave the real answer. Also caught by an existing guard: the longer tool description exceeded the 1024 characters a strict provider accepts. Trimmed to 993 rather than expanded. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 + DECISIONS.md | 2 + .../piing/extensions/organization-intercom.ts | 70 +++++++--- .../piing/test/HireDefaultDepartment.test.ts | 128 ++++++++++++++++++ .../test/IntercomSeamClassification.test.ts | 1 + 5 files changed, 187 insertions(+), 19 deletions(-) create mode 100644 packages/piing/test/HireDefaultDepartment.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6572298..fc3128a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +- **fix(hire): the guidance told agents to omit the department id; the schema required it; the model obeyed the schema and guessed.** `org_hire`'s `departmentId` was a REQUIRED field whose own description opened "DEFAULT: the department YOU head". An agent read that, reasoned correctly that it should omit the field, met a schema that would not permit it, and improvised the most salient name in context — the company's. It obeyed the instrument over the claim, which is the right thing for it to do. + **The field is optional now and the default is real.** An omitted id resolves to the department the caller heads, or failing that the one they sit in — the resolver that already existed and is word-for-word what the description promised. The promise was always implementable; it simply was not implemented. The worked example shows the omitted form first, because an example that keeps passing the field re-teaches the habit the description is trying to correct. + **The company name is still refused on the override path**, and that refusal is now the only thing standing behind an explicitly-passed id, so it is pinned by a test that passes the field deliberately rather than omitting it. Accepting the name as an alias was rejected again and for a sharper reason than before: with a default in place the alias would exist only on the override path, where a mid-level head naming the company means their OWN team — so it would resolve to the impressive-sounding wrong target rather than the semantically right one. + **A sweep asked the general question of every parameter in the file** — does the schema permit what the prose promises? — across `DEFAULT`, `omit`, `omitted`, `optional` and `leave empty`. Exactly one leaf field disagreed, this one. One other hit reads "never omit this field" and is required, so prose and schema agree. A mechanical guard was therefore not added: an instrument for a class of one is furniture. + - **fix(install): the installer no longer prints a curl error on the first line a stranger runs.** A clean install printed `curl: (23) Failure writing output to destination` immediately under "Resolving the latest chief release…", and then completed successfully. The install was never broken; the message was, and it appeared on the very first command anyone runs against this project — where a reader has no reason to read it as anything but a failure. **The cause was a pipeline, not the request.** The release lookup piped curl into `grep -m1`, which exits on its first match; if curl is still writing when it does, curl's write fails and it says so on stderr. The tag had already been captured, which is why everything downstream worked. Whether it appeared at all depended on whether the response outran the pipe buffer, which is why it was intermittent rather than constant — and why it is the kind of thing that reaches a stranger before it reaches the people who built it. **The installer also puts chief on your PATH itself**, instead of printing a line to copy. It writes to `~/.bashrc` and `~/.zshrc` when they exist, creates the one matching your shell when neither does, and **names every file it touched** — a script that edits somebody's dotfiles and does not say which is asking to be distrusted. Running it again changes nothing: a profile that already exports that directory is recognised and left alone, matched on the PATH rather than on an exact line, so a hand-edited variant counts as done. diff --git a/DECISIONS.md b/DECISIONS.md index 1e728d3..933e43e 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1,3 +1,5 @@ +- 2026-08-28 — **A parameter description is a promise about the schema, and the schema is what the model obeys.** A documented default that the schema does not implement does not produce a caller who ignores the prose — it produces one who believes the prose, discovers the field is required anyway, and IMPROVISES a value. That is what happened: "DEFAULT: the department YOU head" over a required field, and an agent that reasoned its way to omitting the field, could not, and filled it with the most salient name in context. The instrument is the schema; the description is a claim about it; they must agree, and where they disagree the claim is what gets believed and the schema is what gets obeyed. The general question worth asking of any tool surface — **does the schema permit what the prose promises?** — is answerable mechanically, which is why the sweep was the substance of this change rather than an appendix to it. Recorded with the sharper reason the company-name alias stays rejected: with a real default in place the alias would exist ONLY on the override path, where a mid-level head naming the company means their own team — so it would resolve to the impressive-sounding wrong target rather than the semantically right one, which is worse than the refusal it would replace. + - 2026-08-28 — **Read your own pushed bytes: a claim that reads as verified is not verified, and the checks do not cover claims.** Three defects in one day were of this shape and none was catchable by any test in the repository: a comment naming THREE adapters when there were eight (a wrong number); a comment citing a guard called `CatchPathsFunnelThroughRefusalResult` that nothing answers to (a wrong referent); and a report that a group of conversions was complete on the strength of three sampled greps (a wrong scope). Each read as though somebody had checked, each was green, and each was found by looking at the pushed artifact rather than by running anything. The rule that follows is cheap and mechanical: after pushing, read what you pushed — the bytes at the SHA, not the diff you intended — and read the prose in it as sceptically as the code, because a comment is the one part of a change that no gate can falsify. Its companion, learned the same day: when a claim is disputed, settle it with the bytes at a named SHA rather than with a commit id, since a commit id proves when you pushed and only the bytes prove what is there. - 2026-08-28 — **A refusal must not lie about whose fault it is, because the label chooses the recovery.** A system fault invites the same call again; a caller error invites a corrected one. So a failure a tool DECIDED must be distinguishable from an exception it SUFFERED, all the way to the surface — and where the distinction is carried by the presence of a field, every adapter that flattens an error has to preserve it or the classification is lost in transit. It was: validation refusals were thrown as plain errors and seven catch adapters dropped them into status-less results, so a whole class of deliberate refusals rendered as crashes. The marker travels ON the error rather than being re-derived, because re-deriving it means matching message text, which is a second parser by another name. Two corollaries recorded with it. The plain error keeps its meaning — an invariant no input should reach IS a system fault and retrying it is right — and the fix is pinned from both sides, since a test asserting only "this is not a system fault" can be satisfied by labelling everything a refusal, which deletes the distinction instead of repairing it. And on the same incident: an ambiguous name is PREVENTED at the parameter that accepts it, never accepted as an alias — an alias binds everywhere the parameter appears, so a convenience at hire becomes a destructive action at remove, which is the wrong-target-from-name-confusion class arriving by a friendlier road. diff --git a/packages/piing/extensions/organization-intercom.ts b/packages/piing/extensions/organization-intercom.ts index 708aa16..8485f12 100644 --- a/packages/piing/extensions/organization-intercom.ts +++ b/packages/piing/extensions/organization-intercom.ts @@ -5884,15 +5884,23 @@ const ADD_DEPARTMENT_PARAMETERS = Type.Object({ }, { additionalProperties: false }); const HIRE_PARAMETERS = Type.Object({ - departmentId: Type.String({ + // OPTIONAL, because the description promises a default and a required field + // cannot deliver one. It was `Type.String` — required — under a description + // opening "DEFAULT: the department YOU head". An agent read the prose, + // reasoned that it should omit the field, met a schema that would not let it, + // and improvised the most salient name in context: the company. It obeyed + // the instrument over the claim, which is the correct thing for it to do. + departmentId: Type.Optional(Type.String({ description: - "Where this person lands. DEFAULT: the department YOU head — a hire joins the team that " - + "asked for it. Name a different one only when the operator named it. This call never " - + "creates a department, and a job title never asks for one: \"hire a Chief of Staff\" is a " - + "hire into your own department, not a new unit. Create a department only when the " - + "operator asked for a department in those words. " - + "The company name or slug is NEVER a department id — the root department's id is in org_roster.", - }), + "Where this person lands. OMIT IT to hire into the department you head — that is the " + + "DEFAULT and it is what you want almost always, because a hire joins the team that " + + "asked for it. Pass one only to override that, and only when the operator named a " + + "different department. This call never creates a department, and a job title never " + + "asks for one: \"hire a Chief of Staff\" is a hire into your own department, not a new " + + "unit. Create a department only when the operator asked for a department in those " + + "words. If you do pass one, the company name or slug is NEVER a department id — the " + + "root department's id is in org_roster.", + })), /** One person, the original shape. */ person: Type.Optional(PERSON_SEED), /** Several people in ONE call — see the batch note in `execute`. */ @@ -7439,7 +7447,7 @@ async function installSubtreeTools( pi.registerTool({ name: "org_hire", label: "Hire an organization person", - description: "Hire one durable worker into an EXISTING department — by DEFAULT the one you head — only after the roster shows no suitable existing person. Send person as real JSON, never a quoted string; use people: [ … ] for several at once. Example: {\"departmentId\":\"engineering\",\"person\":{\"name\":\"Rhea\",\"title\":\"Staff Engineer\",\"mandate\":\"Own the SQLite store.\"}}. name is one short first name; the job goes in title. A NEW DEPARTMENT IS THE OPERATOR'S DECISION AND NEVER YOURS TO INFER: if they asked for one in those words use org_add_department, which makes it and its head together; if they did not, this call is the whole answer. \"Chief of Staff\" and \"Head of Growth\" are TITLES, not requests for a unit. No field asks you to justify anything. Put technology requirements in mandate; a hire does not select skills, extensions, or packages. A new hire comes up on its own; you do not have to start them, and nobody is stopped at creation.", + description: "Hire one durable worker into an EXISTING department — by DEFAULT the one you head, so OMIT departmentId unless the operator named another — only after the roster shows no suitable existing person. Send person as real JSON, never a quoted string; use people: [ … ] for several at once. Example: {\"person\":{\"name\":\"Rhea\",\"title\":\"Staff Engineer\",\"mandate\":\"Own the SQLite store.\"}}; add departmentId only to override. name is one short first name; the job goes in title. A NEW DEPARTMENT IS THE OPERATOR'S DECISION AND NEVER YOURS TO INFER: if they asked for one in those words use org_add_department, which makes it and its head together; if they did not, this call is the whole answer. \"Chief of Staff\" and \"Head of Growth\" are TITLES, not requests for a unit. No field asks you to justify anything. Put technology requirements in mandate; a hire does not select skills, extensions, or packages. A new hire comes up on its own; you do not have to start them, and nobody is stopped at creation.", parameters: HIRE_PARAMETERS, prepareArguments: stringifiedArgumentRepair(context, "org_hire", HIRE_PARAMETERS) as never, async execute(_toolCallId, params) { @@ -7481,9 +7489,20 @@ async function installSubtreeTools( // department 'belfort-brothers-capital'" for a department that simply // did not exist, then followed its remediation sentence into a create // the core refuses. Both halves are derived now, never static. - const hireDenial = departmentScopeDenial(gate.manifest, hiringManager, params.departmentId); + // THE DEFAULT THE DESCRIPTION PROMISES, resolved here rather than + // demanded of the caller: the department this person heads, or failing + // that the one they sit in. That is `authorityRootDepartmentId`, which + // already existed and is character-for-character what the prose says — + // the promise was always implementable, it simply was not implemented. + const departmentId = params.departmentId ?? authorityRootDepartmentId(gate.manifest, hiringManager); + if (departmentId === undefined) { + throw new CallerRefusal( + "Could not determine which department to hire into, and none was given. Pass departmentId naming one from org_roster.", + ); + } + const hireDenial = departmentScopeDenial(gate.manifest, hiringManager, departmentId); if (hireDenial === "unknown-department") { - throw new CallerRefusal(unknownDepartmentMessage(gate.manifest, hiringManager, params.departmentId, "hire into")); + throw new CallerRefusal(unknownDepartmentMessage(gate.manifest, hiringManager, departmentId, "hire into")); } if (hireDenial) { // Name the ACCEPTED path, not just the refusal. Everyone now carries @@ -7491,7 +7510,7 @@ async function installSubtreeTools( // department it merely sits in — and the answer is to grow its own // unit first, never to loosen the scope check. throw new Error( - `'${hiringManager.id}' does not manage department '${params.departmentId}'. ${hiringPathAdvice(gate.manifest, hiringManager)}`, + `'${hiringManager.id}' does not manage department '${departmentId}'. ${hiringPathAdvice(gate.manifest, hiringManager)}`, ); } for (const seed of seeds) { @@ -7501,35 +7520,35 @@ async function installSubtreeTools( // on the operator's own defaults, like everybody else. const request = hireRequest({ slug: gate.slug, - departmentId: params.departmentId, + departmentId, hiringManagerPersonId: hiringManager.id, person: seed as unknown as Record, }); const outcome = await staffingApply(gate, "/v1/org/person/hire", request as unknown as Record, { - action: "hire", departmentId: params.departmentId, personId: request.personId || undefined, name: request.name, + action: "hire", departmentId, personId: request.personId || undefined, name: request.name, }); // A refusal mid-batch reports WHO was already hired. Silently // dropping that list is how an operator retries a batch and gets // duplicates of the people who succeeded the first time. if ("refused" in outcome) { - return routeRefusal("Hire", outcome, { departmentId: params.departmentId, hired }); + return routeRefusal("Hire", outcome, { departmentId: departmentId, hired }); } hired.push({ name: request.name }); } if (hired.length === 1) { const only = hired[0]!; - return toolResult(true, `Hired ${only.name} into '${params.departmentId}'. They come up on their own; they stop on their own once they settle after idling.`, { + return toolResult(true, `Hired ${only.name} into '${departmentId}'. They come up on their own; they stop on their own once they settle after idling.`, { status: "applied", - departmentId: params.departmentId, + departmentId: departmentId, name: only.name, hired, }); } const roster = hired.map((entry) => entry.name).join(", "); - return toolResult(true, `Hired ${hired.length} people into '${params.departmentId}': ${roster}. They come up on their own; each stops on its own once it settles after idling.`, { + return toolResult(true, `Hired ${hired.length} people into '${departmentId}': ${roster}. They come up on their own; each stops on its own once it settles after idling.`, { status: "applied", - departmentId: params.departmentId, + departmentId: departmentId, hired, }); } catch (error) { @@ -8262,6 +8281,19 @@ export function messageWakeDispositionForTest( * error a validation site throws, so the round trip is testable without * driving a whole tool. */ +/** + * The default `org_hire` resolves when `departmentId` is omitted — the one the + * parameter description promises. Exported so BOTH arms of it can be asserted + * without booting a company: the department a head heads, and the department a + * non-head merely sits in. + */ +export function hireDefaultDepartmentForTest( + manifest: IntercomOrganizationManifest, + person: PersonRecord, +): string | undefined { + return authorityRootDepartmentId(manifest, person); +} + export function refusalResultForTest(error: unknown): { details?: Record } { return refusalResult(error) as unknown as { details?: Record }; } diff --git a/packages/piing/test/HireDefaultDepartment.test.ts b/packages/piing/test/HireDefaultDepartment.test.ts new file mode 100644 index 0000000..c945973 --- /dev/null +++ b/packages/piing/test/HireDefaultDepartment.test.ts @@ -0,0 +1,128 @@ +/** + * THE SCHEMA MUST PERMIT WHAT THE DESCRIPTION PROMISES. + * + * `org_hire`'s `departmentId` was a REQUIRED field whose own description opened + * "DEFAULT: the department YOU head". An agent read that, reasoned correctly + * that it should omit the field, met a schema that would not allow it, and + * improvised the most salient name in context — the company's. + * + * It obeyed the instrument over the claim, which is the right thing for it to + * do. The description was a promise about the schema that the schema did not + * implement, and prose is the one part of a tool surface no gate falsifies. + */ +import { isNullish } from '@test/support/Nullish' +import type { IntercomOrganizationManifest, PersonRecord } from '@test-assets/organization-intercom' +import { + departmentScopeDenial, + hireDefaultDepartmentForTest, + unknownDepartmentMessage +} from '@test-assets/organization-intercom' +import { describe, expect, test } from 'vitest' + +function company(): IntercomOrganizationManifest { + return { + schemaVersion: 1, + kind: 'organization', + slug: 'acme-capital', + name: 'Acme Capital', + rootDepartmentId: 'executive', + departmentOrder: ['executive', 'engineering'], + peopleOrder: ['chief', 'eng-head', 'worker'], + departments: { + executive: { + id: 'executive', + name: 'Executive', + headPersonId: 'chief', + parentDepartmentId: undefined, + purpose: 'Run the company.', + state: 'active' as const + }, + engineering: { + id: 'engineering', + name: 'Engineering', + headPersonId: 'eng-head', + parentDepartmentId: 'executive', + purpose: 'Ship it.', + state: 'active' as const + } + }, + people: { + chief: person('chief', 'Ada', 'executive'), + 'eng-head': person('eng-head', 'Priya', 'engineering'), + worker: person('worker', 'Dana', 'engineering') + } + } +} + +function person(id: string, name: string, departmentId: string): PersonRecord { + return { + id, + name, + title: 'Person', + kind: 'worker' as const, + departmentId, + employmentState: 'active', + createdAt: '2026-01-01T00:00:00.000Z' + } +} + +describe('a hire with no departmentId lands in the caller’s own department', () => { + test('a HEAD gets the department they head', () => { + const manifest = company() + + expect(hireDefaultDepartmentForTest(manifest, manifest.people['eng-head'])).toBe('engineering') + expect(hireDefaultDepartmentForTest(manifest, manifest.people.chief)).toBe('executive') + }) + + /** + * THE OTHER ARM. A person who heads nothing still has a department — the one + * they sit in — and the resolver has always had both branches. Asserting only + * the head case would pass against a resolver that returned the headed + * department or nothing. + */ + test('a NON-head gets the department they sit in', () => { + const manifest = company() + + expect(hireDefaultDepartmentForTest(manifest, manifest.people.worker)).toBe('engineering') + }) + + test('the resolved default is a department this person may actually hire into', () => { + // The default is only worth having if it survives the scope check the hire + // then applies to it — otherwise omitting the field would trade a guess for + // a refusal. + const manifest = company() + const head = manifest.people['eng-head'] + const resolved = hireDefaultDepartmentForTest(manifest, head) + if (isNullish(resolved)) throw new Error('the default must resolve for a head') + + expect(departmentScopeDenial(manifest, head, resolved)).toBeUndefined() + }) +}) + +describe('the override path still refuses a company name', () => { + /** + * REACHABILITY, not merely behaviour. With a default in place, the refusal is + * the only thing standing behind an EXPLICIT departmentId — so this drives + * the explicit path on purpose. The fixture passes the field rather than + * omitting it, or the test would quietly become a default-path test the day + * the default landed and stop guarding anything. + */ + test('an explicit company name is still refused, and the refusal names the root id', () => { + const manifest = company() + const explicitlyPassed = manifest.slug + + expect(departmentScopeDenial(manifest, manifest.people.chief, explicitlyPassed)).toBe( + 'unknown-department' + ) + + const refusal = unknownDepartmentMessage( + manifest, + manifest.people.chief, + explicitlyPassed, + 'hire into' + ) + expect(refusal).toContain("The root department id is 'executive'") + expect(refusal).toContain('acme-capital') + expect(explicitlyPassed).not.toBe(hireDefaultDepartmentForTest(manifest, manifest.people.chief)) + }) +}) diff --git a/packages/piing/test/IntercomSeamClassification.test.ts b/packages/piing/test/IntercomSeamClassification.test.ts index 08b27be..5500a1b 100644 --- a/packages/piing/test/IntercomSeamClassification.test.ts +++ b/packages/piing/test/IntercomSeamClassification.test.ts @@ -281,6 +281,7 @@ const CLASSIFICATION: Readonly> = { // Both are B: they are about what a person is SHOWN and what an agent may // type, not about where a decision lives. recipientsForTest: 'B', + hireDefaultDepartmentForTest: 'B', // The refusal classification: which failures are the CALLER's and which are // the system's. Presentation, because the whole subject is what the card // tells a reader about whose fault it is.