fix: fail-closed intent-birth, scope-runner new-work path, fresh-session handoff (2.5.48) - #676
Conversation
fc2611f to
7030ad1
Compare
leandrodamascena
left a comment
There was a problem hiding this comment.
This change introduces the intended fresh-session boundary, but the implementation is inconsistent across harnesses and corrupts session audit attribution on the exact handoff path. It also leaves two mutation paths able to create undescribed intents. Please address the inline findings before merge.
| // concrete command (Claude: `/clear`). | ||
| if (flags.newIntent) { | ||
| return printDirective( | ||
| `${runCmd} to start the new intent${cost}.${labelHint} Then STOP, do NOT re-run \`next\` in this session. ` + |
There was a problem hiding this comment.
[P1] Keep the fresh-session handoff consistent across harnesses
The shared directive now tells every harness to stop after intent-birth, but only Claude’s orchestrator skill was updated. Codex, Kiro, Kiro IDE, and OpenCode still instruct the conductor to re-run next in the same session, preserving the stale-context bug this change intends to fix. The shared message also says /aidlc, while Codex uses $aidlc. Please update every harness-specific skill and tailor the resume command.
| return printDirective( | ||
| `${runCmd} to start the new intent${cost}.${labelHint} Then STOP, do NOT re-run \`next\` in this session. ` + | ||
| `This is a NEW, unrelated intent, and the current session still carries the previous intent's context. ` + | ||
| `Tell the user to start a fresh session (in Claude Code, \`/clear\`; or restart the CLI), then run \`/aidlc\` to begin the new intent with a clean slate. ` + |
There was a problem hiding this comment.
[P1] Attribute SESSION_ENDED to the session’s original intent
intent-birth switches the active-intent cursor before the user runs /clear. The SessionEnd hook resolves its audit destination from that cursor, so the old session’s SESSION_ENDED event is written to the newly created intent. I reproduced an old intent containing SESSION_STARTED while the new intent received SESSION_ENDED: clear. Please preserve the original session attribution or clear before switching the cursor, with an integration test covering the complete handoff.
| // current stage". The freeform new-work text rides in flags.intent (the same | ||
| // slot Branch 9a threads as the description). | ||
| // birth print that performs it. Unlike the fresh-start tail, the new-intent | ||
| // directive tells the conductor to STOP after birth and hand off to a fresh |
There was a problem hiding this comment.
[P2] Reject --new-intent without a description
next --new-intent currently emits a valid birth command even when no new-work text was supplied. Following it creates a generic scope-named intent and repoints the active cursor, despite this branch claiming that distinct work was confirmed. Require a nonblank description before emitting the mutation.
| // print directive (`next` / `next --new-intent`) always names `--scope`, and | ||
| // the init runner forwards a `--arguments` description. This never trips a | ||
| // legitimate call. | ||
| if (!flags.scope && !flags.arguments && !flags.label) { |
There was a problem hiding this comment.
[P2] Treat missing flag values as absent
The new fail-closed guard checks only truthiness, but parseArgs() represents a valueless option as "true". Therefore, intent-birth --arguments bypasses the guard and creates an intent such as 260803-true; --label behaves similarly. Validate that value-bearing flags have real values before allowing the mutation.
|
Addressed all four review findings in
Verification:
|
ef26ad7 to
fadf5d1
Compare
leandrodamascena
left a comment
There was a problem hiding this comment.
Two correctness issues remain:
-
core/tools/aidlc-utility.ts:202
.current-sessionidentifies the most recently started session, not the session invokingintent-birth. With concurrent pre-workflow sessions A and B, a birth initiated by A can stamp B, causing later session events to be attributed to the wrong intent. -
core/hooks/aidlc-session-start.ts:178
/aidlcand$aidlcare skill invocations, not shell commands. Joining them with&&makes the second invocation trailing arguments, so cross-space rebind may switch spaces without selecting the intended intent. Emit two sequential instructions instead.
Validation: 73 focused tests, package parity, typecheck, and lint passed.
|
Addressed both new review findings in
Verification:
|
leandrodamascena
left a comment
There was a problem hiding this comment.
The latest commit addresses the two previous findings, but two fresh-session correctness issues remain, and CI is currently red.
1. P1: Stop enforcement defeats the fresh-session handoff
After --new-intent creates the second intent, the directive tells the current session to stop. However, aidlc-stop.ts resolves the newly active intent and runs next against it. Because that intent has pending work, the hook returns decision: block and forces the old, context-polluted session back into the workflow.
References:
core/tools/aidlc-orchestrate.ts:540-545core/hooks/aidlc-stop.ts:875-882core/hooks/aidlc-stop.ts:953-966core/hooks/aidlc-stop.ts:1074-1095
I reproduced this end-to-end after two successful births: the second intent became active, then Stop returned a forwarding-loop block instead of allowing the requested reset.
Please make Stop resolve the invoking session’s stamped intent or recognize the explicit post-birth handoff boundary. Add a regression test that performs the second birth and invokes the real Stop hook before firing SessionEnd.
2. P1: modern Kiro IDE sessions cannot emit SESSION_ENDED
Modern SessionStart forwards the host’s real session_id, but the payload-free Kiro IDE SessionEnd path always forwards kiro-ide-legacy-current. For a UUID-backed workflow, the core hook refuses fallback from that unstamped synthetic ID, so the modern session’s SESSION_ENDED event and heartbeat are silently dropped.
References:
harness/kiro-ide/hooks/aidlc-kiro-adapter.ts:346-355harness/kiro-ide/hooks/aidlc-kiro-adapter.ts:538-545core/hooks/aidlc-session-end.ts:48-67
The existing test covers only the legacy synthetic-session path. Please add a modern SessionStart → birth binding → payload-free agentStop test and preserve a session identity that the SessionEnd hook can resolve.
3. P2: the committed unit suite is red
classifyTerminalCommand() intentionally returns null for intent birth, but the workspace parser parity test still requires a non-null classification.
Reference: tests/unit/t229-workspace-parser.test.ts:250-255
This is also the failure reported by the current smoke+unit CI job.
Verification
- Focused unit slice: 175 passed, 1 failed (
t229-workspace-parser). - Intent-birth integration: 42 passed.
bun scripts/package.ts --check: passed for all harnesses.git diff --check: passed.- The PR remains conflicting with current
v2, including authored conflicts inaidlc-orchestrate.tsandt165-intent-birth-p4.test.ts.
I recommend keeping CHANGES_REQUESTED until both handoff paths are corrected, the unit suite is green, and the branch is rebased.
7b5124d to
de77a19
Compare
|
Addressed the latest review findings and rebased onto current
Verification:
|
de77a19 to
bfb7a26
Compare
leandrodamascena
left a comment
There was a problem hiding this comment.
The latest commit resolves the previously blocking findings:
- The Stop hook now recognizes the exact post-
intent-createfresh-session handoff using a one-shot session receipt. - Kiro IDE no longer maps its modern per-turn
Stopevent toSESSION_ENDED. - The workspace parser regression is fixed.
- The branch is current with
v2, mergeable, and package output is synchronized.
I am comfortable approving this revision.
Two non-blocking P2 follow-ups remain for Kiro IDE session identity.
P2 follow-up: preserve the legacy synthetic session during intent creation
harness/kiro-ide/hooks/aidlc-kiro-adapter.ts:491
The legacy Kiro IDE channel has no host-provided session_id, so SessionStart records the synthetic identity kiro-ide-legacy-current. However, the rebuild-stage-graph forwarding path includes a session ID only when ide.sessionId exists. Consequently, a successful legacy intent-create reaches bindCreatedIntentToInvokingSession() without any session identity, so the hook cannot record initial ownership or a second-intent handoff receipt.
I reproduced the first-create path and confirmed that the expected synthetic session stamp was missing.
Suggested follow-up: forward ide.sessionId when available and otherwise use the remembered synthetic session identity.
P2 follow-up: use the Stop event session identity
harness/kiro-ide/hooks/aidlc-kiro-adapter.ts:106,616
Modern Kiro IDE Stop payloads contain session_id, but continue-workflow is excluded from INPUT_TARGETS. The adapter therefore ignores the event-local identity and forwards the workspace-global last-started session instead. With two chats in the same workspace, the most recent SessionStart can overwrite that shared marker. A Stop from S1 can then be evaluated as S2, leaving the exact S1 handoff receipt unconsumed.
I reproduced this with two modern sessions: the receipt correctly described the S1 intent boundary, but it remained after a Stop carrying session_id: "S1".
Suggested follow-up: parse and prefer the Stop payload session_id, retaining the remembered identity only as the legacy fallback.
These issues are limited to Kiro IDE compatibility and session-accounting behavior. They do not corrupt the durable intent record, and modern Kiro IDE Stop is advisory rather than blocking, so I do not consider them merge blockers.
Validation
- 145 focused tests passed across six files.
bun scripts/package.ts --checkpassed for every harness.bun run typecheckpassed.bun run lintexited successfully.git diff --check origin/v2...HEADpassed.- The worktree remained clean at
bfb7a261.
…scope-runner new-work path (2.5.24)
bfb7a26 to
d2c0c86
Compare
Problem
Three compounding defects affect starting and finishing intents:
intent-birth(no --scope, no --arguments, no --label) silently minted a garbage intent: default scope, empty [Project description], an intents.json row, and a repointed active-intent cursor - manual cleanup required. Verified empirically on upstream before fixing: exit 0, a minted default-scope intent from a bare call.nextcorrectly returns done - and the runner stopped there, with no instruction that unrelated new work goes throughnext --new-intent. Dead end.What changes
next --new-intent --scope <the runner's baked scope>. Both completed-workflow done directives now carry a NEW_WORK_HINT so the escape hatch surfaces at exactly the moment it is needed.Testing
Note for maintainers: version/CHANGELOG re-bump expected when landing after sibling PRs.