fix(des-commit)!: emit Task-Id trailer required by stop hook - #86
Conversation
The SubagentStop commit verifier greps for BOTH `Step-Id: {step_id}` and
`Task-Id: {project_id}` on the same commit (`git log --grep ... --all-match`),
but `des-commit` only ever appended `Step-Id:` and had no input for the task
id. A crafter following the shipped nw-execute COMMIT template verbatim was
therefore guaranteed a `COMMIT_NOT_VERIFIED` block on its first commit.
des-commit now takes `--task-id` and appends both ids as ONE well-formed final
trailer block (joined to an existing trailer paragraph instead of always
opening a new one), so `git interpret-trailers` / `%(trailers)` also see both
keys — previously an author-placed `Task-Id:` landed in a separate paragraph
and was dropped by trailer parsing.
`--task-id` is REQUIRED, not optional-with-default: an optional flag with a
silent default reproduces the exact failure this fixes whenever it is omitted,
only later and in the hook rather than at the producer. Required means the
failure is a usage error naming the missing input.
BREAKING CHANGE: `des-commit` invocations without `--task-id` now exit 2. Any
existing caller must pass the bare feature id (`44`, not `nWave-ai#44`) — that literal
is what the verifier greps for. A maintainer who prefers optional-with-env-
default (e.g. reading DES-PROJECT-ID from the dispatch prompt) can relax the
flag without touching the trailer-writing logic.
The nw-execute COMMIT template (skill + command) now documents both trailers
and the bare-id value format, mirroring what the verifier greps.
Issue: nWave-ai#78
|
The following files are on the sensitive-file denylist and cannot be
These files are typically managed by the release pipeline or contain This PR remains open for discussion. No mirror branch has been created. |
|
Correction to this PR's own description, not to the diff. I originally wrote that the verifier greps for "the bare feature id ( The code, the template and the tests were always consistent on this; only my description was misleading. The body is now corrected. |
|
Question on the This PR edits the marketplace payload under I kept the payload edits rather than stripping them, deliberately, because the asymmetry favours it: if the payload IS regenerated, these hunks are harmless noise that your pipeline overwrites; if it is NOT, stripping them would ship a marketplace copy contradicting the source — which is exactly the divergence issue #83 describes. If the payload is generated, say so and I will drop those hunks from this PR. Nothing else in the diff depends on them. |
plugins/** is on the repository's sensitive-file denylist and cannot be changed through the external PR flow -- the mirror bot refuses to create a nwave-dev branch while those paths are touched. The payload copies of commit.py and execute.md are reverted to their committed state. They therefore still lack --task-id. If plugins/ is regenerated at release time this resolves itself; if it is not, the marketplace channel needs the same change applied maintainer-side. Issue: nWave-ai#78
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
Review found the first pass recreated issue nWave-ai#78 inside its own fix. The trailer guard was a substring test over the whole message: if "Task-Id:" not in body: so any message merely MENTIONING the key suppressed the real trailer. A commit message like "fix: correct Step-Id: parsing in the verifier" produced a commit with no Step-Id trailer at all, which the SubagentStop verifier then rejected with COMMIT_NOT_VERIFIED and no indication why. That is the exact failure this CLI change exists to remove. Detection now parses the final trailer block, which is the only place git's own trailer machinery looks. A key mentioned in the subject, in prose, or in a code sample is text, not a trailer. IF the final trailer block already carries the key with a DIFFERENT value, the CLI now refuses (exit 2) instead of silently keeping one of the two. Guessing which value the verifier should see is how the original bug reached production. A key already present with the SAME value is left alone and never duplicated. Also fixed in the same pass: - Empty or whitespace-padded --step-id / --task-id are rejected. They passed argparse and produced a commit the verifier could never match. - The post-commit `git reset` return code was discarded, so a failed resync of the shared index reported success. It now warns, naming the recovery command. It does NOT fail the run: the commit already exists at that point, and exit 1 would tell the caller to retry a commit that succeeded. - Commit errors now name the owned paths that were attempted. - --task-id help said "bare value (e.g. 44, not nWave-ai#44)", implying an issue number. subagent_stop_service states project_id IS feature_id, so the value is the feature slug (auth-upgrade). Help and usage example corrected. Four tests cover the branches that carried the defect and previously had none: prose mention does not suppress the trailer, a conflicting trailer is refused with no commit created, a matching trailer is accepted without duplication, and a blank id is a usage error. Issue: nWave-ai#78
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
3a0a256 to
1367005
Compare
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
…dness The headline Task-Id test searched the whole commit message for the substring "Task-Id: 44". That passes even when the key lands where git's trailer parser cannot see it — the exact defect issue nWave-ai#78 fixes — so it gave false confidence. It now reads the value back through %(trailers:key=Task-Id,valueonly), as its sibling already did. The help-contract test claimed to check that --task-id is advertised as required but only checked that the string appeared. It now asserts on the usage line, where argparse distinguishes required from optional by bracketing; flipping required=False on the option makes it fail. Fixture task ids move from "44" to the slug "auth-upgrade": the Task-Id value is the feature-id slug read from execution-log.json, not an issue number, so the tests now document the real semantics. Issue: nWave-ai#78
|
Pushed The headline test was asserting the wrong thing. Fixtures corrected to the real semantics. The tests used The help-contract test now asserts requiredness, and that assertion was mutation-checked. It previously asserted only that Deliberately left out of this PR: 113 passed, 1 pre-existing xfail. |
|
This change does not apply cleanly onto the matching private base. No action is required from you right now. We'll comment again once |
2 participants
Why
The two halves of the commit contract disagreed, and neither side named the value that would satisfy the other. The SubagentStop verifier greps for both
Step-Id: {step_id}andTask-Id: {project_id}on the same commit (git log --grep … --all-match), whiledes-commit— the producer the DES template mandates — only ever appendedStep-Id:and had no input for a task id at all. A crafter following the shippednw-executeCOMMIT template verbatim was therefore guaranteed aCOMMIT_NOT_VERIFIEDblock on its first commit, and could only escape by hand-embedding an undocumented trailer discovered through trial and error.What changed
src/des/cli/commit.py— new required--task-id; both ids are appended as one well-formed trailer block.nWave/skills/nw-execute/SKILL.md,nWave/tasks/nw/execute.md— the COMMIT template now documents both trailers and the value format the verifier greps for.tests/des/integration/test_des_commit_cli.py,tests/des/unit/cli/test_cli_help_contract.py— coverage for the trailer, the dual-grep satisfaction, and the usage error.Not changed: the
plugins/**copies ofcommit.pyandexecute.md. That tree is generated byscripts/build_plugin.pyand is on the mirror bot's sensitive-file denylist — including it blocked the nwave-dev mirror. The marketplace payload therefore still lacks--task-idand needs a maintainer-side regen.A second defect found while fixing the first
Appending
Task-Id:as its own paragraph is not enough:git interpret-trailersand%(trailers)only parse the final trailer paragraph, so an author-placedTask-Id:in a separate block was silently dropped by trailer parsing even when it was visibly present in the message. The fix joins both keys into the existing trailer paragraph rather than opening a new one. This is why the change is slightly larger than "add a flag".Why
--task-idis required rather than optionalThe load-bearing property named in #78 is that the failure must not be guessable-wrong. An optional flag with a silent default reproduces exactly the failure being fixed whenever the flag is omitted — only later, and in the hook rather than at the producer, which is the worse of the two places to discover it. Required means omission is a usage error that names the missing input.
This is the reviewable judgment call in this PR, and it is reversible: a maintainer who prefers optional-with-env-default (say, reading the project id from the dispatch prompt) can relax the flag without touching the trailer-writing logic, which is the part that carries the real fix.
Test plan
./.venv/bin/python -m pytest tests/des/integration/test_des_commit_cli.py tests/des/unit/cli/test_cli_help_contract.py -q— 28 passed.tests/descompared within the same worktree and the same virtualenv, change reverted vs applied: 37 failures → 21. Zero regressions; 16 tests move from red to green, including all five newTestDesCommitTaskIdTrailercases (they fail with the CLI change reverted, which is the RED proof).--grep … --all-matchis asserted directly against a real commit instead.A note on the absolute failure counts: this repo's
tests/deshas pre-existing failures unrelated to this change (12 on a pristine1d0f13cin my environment). Cross-environment counts are not comparable, becauseuv syncfails on a fresh clone here —pyproject.tomlforce-includeslib/python/des, which is untracked in this repository — so the suite has to be run from a hand-built virtualenv. The base-vs-head comparison above is therefore done inside one environment, which is the only comparison that means anything.Focus areas
--task-id. This is the one decision worth your time; see the section above. If you want it optional, say so and the trailer logic stays as-is.--messagealready ends in a trailer paragraph, and when it ends in prose. Both are covered by tests, but this is the subtle part.project_idrecorded inexecution-log.json— whichsubagent_stop_service.pydocuments as being the feature id, e.g. the slugauth-upgradeused in the template's own exemplar. It is not an issue number. The template says{feature-id}and showsTask-Id: auth-upgrade; if the intended format is different, the template and the tests both need to change with it.plugins/**payload copy diverges fromsrc/until it is regenerated (see Nothing tells contributors plugins/nw is generated output #97).Risk / rollback
Blast radius is every DELIVER commit path. Any existing caller invoking
des-commitwithout--task-idnow exits 2 with a usage error — loudly, not silently, and at the producer.Revert with
git revert <sha>; nothing persists outside the commit message text, so there is no migration or data to undo.BREAKING CHANGE:
des-commitnow requires--task-id. Callers must pass the feature id recorded asproject_idin the execution log.Closes #78