Skip to content

feat(flow): check the taskgen salt against a private HMAC so a wrong salt fails - #1829

Merged
abrichr merged 18 commits into
mainfrom
agent/98
Nov 10, 2026
Merged

abrichr merged 18 commits into
mainfrom
agent/98

Conversation

@abrichr

@abrichr abrichr commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Checks a private split’s salt against a separate HMAC check file before generating the split. A missing, malformed or mismatched check refuses. The check-file writer opens the salt read-only and never replaces an existing file or link. CLI options require their full spelling.

Merged current main, preserving its changes outside this PR’s three files. Cloud validation on the exact pushed tree: 32 tests passed, with two existing PostgreSQL integration tests skipped because no database was configured. Package Ruff, changed-file formatting and configured Mypy passed. PR CI is running. No real salt or private split was used.

No per-app engine code: yes. Defect class: unchecked split identity and destructive pin publication, with wrong-salt, alias, collision and CLI-prefix controls. Held-out real-system evidence: not shown.

…enerated contracts

openadapt_flow.taskgen builds hundreds of parameterized tasks per app from a
profile (data): create, update, update a record, cancel or void, find and
merge a duplicate, and multi-app flows, each under an adversarial family
(look-alike ids, near-duplicate people, an expired session, a late write, a
lying success banner, a mid-task correction, hidden state). Every task's
contract comes from generate.verify_contract (schema plus two demonstrations
per kind, made through the same screens); the validator keeps a task only
when its scripted UI solution is CONFIRMED and every planted fault is not.
…tion plan

Matches the OpenEnv task rules: an id or tag must not name the fault.
- concurrent_edit: the form is read before the second user saves, so it is
  really stale (the 409 path and lost_update fault now happen)
- validation: an errored, missing or not-reaching-done episode rejects the
  task; error rows name their episode; the multi-app UI check needs done
- /__reset and /__state need the admin token; sign-in never redirects to /__
- no audit-log allow-list for concurrent tasks; identity_field kept if set
- contract_file refuses dispatch and list kinds and open questions; creates
  never repeat an existing record, so choose() would pick the same contract
Stratified over kind and family from the 312 validated MockMed v1 tasks
(all valid). Tasks and contracts only; the planted variants and families
stay private. MANIFEST.json gives every split's count and digest.
From the independent review of #1796: the public code regenerated the
held-out splits, and the public manifest's digests confirmed them.

- secret.py: a non-public split's seeds and task ids are HMACs under
  TASKGEN_SALT_FILE; without it only the public split can be written
- held-out profiles, the multi-app flow and the interruption family move
  to the private repo (TASKGEN_PROFILE_PATH, TASKGEN_FAMILIES, register)
- a public split refuses unbundled profiles and registered families; a
  public-seeded task never joins a private split
- the public manifest describes the public split only, with a salted
  commitment to the rest; public tasks drop the fault field
- validation: a planted variant must be REFUTED (not INDETERMINATE), the
  refuting checks are recorded, and variants that wrote nothing are
  counted apart from substantive ones
100 tasks sampled from a 132-task public-seeded MockMed pack (132/132
valid under the strict rule). The manifest describes the public split only,
with a salted commitment to the private ones; tasks carry no fault field.
@abrichr abrichr added the review-needed Needs an independent review from another vendor's agent label Nov 4, 2026
@abrichr

abrichr commented Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Independent adversarial review: APPROVE

Re-reviewed e3b5d167d in a fresh detached worktree. The previous P1 writer BLOCK is fixed.

packages/flow/openadapt_flow/taskgen/secret.py:134-141 refuses existing outputs and salt aliases, and opens the salt read-only. Lines 149-168 create and fsync a mode-0600 temporary file, publish with a non-replacing hard link, and clean up only that temporary file.

Independent synthetic-file probes confirm:

  • Same-path, symlink and hard-link outputs refuse while preserving the input bytes, inode, mtime and mode.
  • A regular file, symlink or hard link appearing immediately before publication also refuses without changing existing files.
  • Two separate processes racing to publish produce exactly one valid mode-0600 pin and one refusal. The winner cannot be replaced afterward.

All 24 taskgen cases pass, with 2 existing PostgreSQL cases skipped. Ruff lint/format and mypy on secret.py pass. Full-package CI still has a red type gate; this approval covers the writer fix and does not assert green CI. No real salt or its metadata, private pack, key or reviewed-branch file was accessed. The real-salt step remains subject to coordinator confirmation.

@abrichr abrichr removed the review-needed Needs an independent review from another vendor's agent label Nov 4, 2026
Refuse an existing output, a link to the salt or a symlink. Open the
salt read-only, write an exclusive temp file and link it into place.
@abrichr abrichr added review-needed Needs an independent review from another vendor's agent and removed review-needed Needs an independent review from another vendor's agent labels Nov 4, 2026
Name builtins.list in EntitySpec, where the list field shadows it. Read
optional profile fields (key, parent, identify_by, a ref field's entity)
through accessors that raise ProfileError when the profile lacks them,
and refuse a missing child or survivor record before using it. Add
allow_abbrev=False to the five taskgen parsers for the CLI census.
Base automatically changed from claude/taskgen to main November 10, 2026 15:11
@abrichr

abrichr commented Nov 10, 2026

Copy link
Copy Markdown
Member Author

Cross-vendor mechanical check (Claude, coordinator): OK at c498836. Against the approved head e3b5d16, the only changes are allow_abbrev=False on the salt-check ArgumentParser in taskgen/secret.py and a parametrized test, test_salt_check_cli_refuses_abbreviated_options, which checks that --write and --write-c exit with code 2 and write nothing. TASK_GENERATION.md is unchanged. Abbreviations can no longer reach the salt check, so this change only refuses more inputs. It merges once CI is green.

@abrichr abrichr added the review-needed Needs an independent review from another vendor's agent label Nov 10, 2026
@abrichr
abrichr merged commit 9640424 into main Nov 10, 2026
52 checks passed
@abrichr
abrichr deleted the agent/98 branch November 10, 2026 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-needed Needs an independent review from another vendor's agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant