Conversation
There was a problem hiding this comment.
🟡 Changes recommended
It updates release metadata surfaces (version constant/README badge/CHANGELOG entry) which repository policy reserves for a dedicated release-preparation PR, and there are a couple of smaller doc/comment inconsistencies to address.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR routes the orchestrator’s public verbs (park, team-board) from the /aidlc command surface to the orchestrator engine (instead of the freeform “new work” funnel), aligning behavior with documented CLI verbs and preventing accidental intent-creation prompts.
Changes:
- Add
ORCHESTRATOR_VERBSand ensure leadingpark/team-boardremain on the engine path (not off-band harness terminal routing). - Extend
nextflag parsing/dispatch to emit terminalprintdirectives forparkandteam-board, and emit a usage error for unsupportedteam-boardargs / bareunpark. - Add/update unit tests and documentation entries for the new routing behavior.
File summaries
| File | Description |
|---|---|
| tests/unit/t178-classify-terminal-command.test.ts | Adds coverage asserting orchestrator verbs stay on engine path and exports are pinned. |
| tests/unit/t114-orchestrate-next.test.ts | Adds tests for next park, next team-board arg allowlist, and unpark error behavior. |
| tests/.coverage-registry.json | Registers coverage unit for ORCHESTRATOR_VERBS. |
| tests/.coverage-ratchet.json | Updates ratchet baseline for increased covered function count. |
| README.md | Updates version badge to 2.8.3. |
| harness/kiro/hooks/aidlc-kiro-adapter.ts | Exempts orchestrator verbs from the Kiro guard’s “bare-advancing” classification. |
| docs/guide/12-cli-commands.md | Documents /aidlc park and /aidlc team-board in quick reference and detailed sections. |
| docs/guide/11-session-management.md | Adds a session-management note pointing users to /aidlc park and /aidlc --resume. |
| core/tools/aidlc-version.ts | Bumps framework version constant to 2.8.3. |
| core/tools/aidlc-orchestrate.ts | Implements Branch routing for park / team-board and allowlisted args parsing. |
| core/tools/aidlc-lib.ts | Exports ORCHESTRATOR_VERBS and keeps classifyTerminalCommand returning null for them. |
| CHANGELOG.md | Adds a 2.8.3 changelog entry describing the behavior change and upgrade command. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a7515ff to
ffff65b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
team-board is read-only but currently still triggers the engine-touch marker (affecting Stop-hook behavior), and the new /aidlc park docs conflict with the current “unit scoped park” behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
ffff65b to
adfc3af
Compare
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved critical --space path-validation issue and moderate routing/guard parity issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
core/tools/aidlc-lib.ts:1322
- This unconditional early return disagrees with
parseNextFlagsfor non-sole park forms such as["park", "--status"]: the engine's parser scans the later read-only flag and routes--statusthrough Branch 1, while this classifier returns null. That breaks the documented engine/classifier parity; only treatteam-board(which owns all trailing args) and a soleparkas orchestrator-owned here.
core/tools/aidlc-orchestrate.ts:4310 - These values come directly from the user's argv but are interpolated into the shell command without quoting. A value such as
$(...)can execute during the conductor's follow-up command, and names containing spaces are split into extra arguments. Quote each non-flag value with the existingshellArghelper (or preserve the argv structurally) before joining the directive.
const extra = flags.orchestratorVerbArgs && flags.orchestratorVerbArgs.length > 0
? ` ${flags.orchestratorVerbArgs.join(" ")}`
: "";
core/tools/aidlc-orchestrate.ts:1657
- These checks accept any non-flag token as a selector and preserve it verbatim. The new
/aidlcpath can therefore emit a command containing shell metacharacters, and--space ../../outsideis later passed asselectedSpaceintolistIntents/path helpers, allowing traversal outsideaidlc/spaces. Validate the space name with the shared slug validator before forwarding, and shell-quote both selector values in the generated command.
const value = args[j + 1];
if ((t === "--space" || t === "--intent") && value !== undefined && !value.startsWith("-")) {
flags.orchestratorVerbArgs.push(t, value);
docs/guide/12-cli-commands.md:285
- This says
/aidlc parkprints the checkout-local JSON object, but the routed command callshandlePark(), which capturesspawnState()stdout and emits only the outerparkeddirective; thecheckout_localresult is discarded for a Unit-scoped checkout. Either relay that state result or document the directive that the command actually returns.
**Behavior:** The engine routes the verb to `aidlc park`, which emits `WORKFLOW_PARKED`, records the park marker in the state file, and reports the stage it parked at. No stage is advanced and nothing is marked complete. Parking is refused when no workflow is active or the workflow is already Completed. In a Unit-scoped team checkout (a Construction worktree carrying a Unit scope stamp) the same command parks that Unit locally instead: it writes a checkout-local Unit park marker, leaves the shared workflow state untouched, and prints `{"parked": true, "unit": ..., "checkout_local": true}`. Resume with `/aidlc --resume`, which clears whichever marker applies and continues. The verb is sole-token: `park` inside a longer sentence is treated as a description of work, so ask the conductor to park in prose or type the bare verb.
harness/kiro/hooks/aidlc-kiro-adapter.ts:504
- This exempts every command whose first token is
park, butparseNextFlagsonly treats a soleparkas the orchestrator verb;next park <description>is intentionally freeform. With a fresh same-turn latch, the Kiro backstop will therefore let that bare advancingnextthrough, allowing the freeform path to run instead of blocking the accidental follow-up. Restrict the exemption to a solepark(while keepingteam-boardleading-token based because its branch owns all trailing tokens).
!ORCHESTRATOR_VERBS.has(nextArgs[0]) &&
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
adfc3af to
ca0ed79
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings remain in direct team-board argument validation and the Kiro park guard.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
harness/kiro/hooks/aidlc-kiro-adapter.ts:504
- This membership check exempts every command whose first freeform word is
park, includingnext park the car, from the Kiro same-turn roll-forward guard. The engine only treats a soleparkas the park route; longer descriptions intentionally remain freeform, so a freeform request can bypass the guard after a read-only latch and advance instead of being blocked. Restrict the exemption to the actual sole-tokenparkroute (while retainingteam-board's leading-token route), or share the engine's parser predicate.
!ORCHESTRATOR_VERBS.has(nextArgs[0]) &&
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
ca0ed79 to
a236316
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The direct team-board path must reject unknown and positional arguments before approval.
Review details
Suppressed comments (1)
core/tools/aidlc-orchestrate.ts:9175
- The direct
aidlc-orchestrate team-boardpath still ignores arbitrary trailing tokens: for example,team-board --bogusorteam-board --space one junkreaches board rendering successfully, whereas the newnext team-boardparser rejects them. This violates the documented allowlist and lets malformed commands report success; parse the complete argv here (reject unknown/positional tokens and ambiguous duplicate selectors) before resolving the board.
const flagValue = (name: string, grammar: RegExp): string | undefined => {
const index = args.indexOf(name);
if (index < 0) return undefined;
const value = args[index + 1];
if (!value || value.startsWith("-")) {
throw new Error(`team-board ${name} requires a value.`);
}
// The value is joined into record paths below; refuse anything outside the
// name grammar so a selector cannot read outside aidlc/spaces.
if (!grammar.test(value)) {
throw new Error(`team-board ${name} "${value}" is not a valid name.`);
}
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
a236316 to
d540946
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate review findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
core/tools/aidlc-orchestrate.ts:4093
- This marker exclusion does not make a valid
/aidlc team-boardturn conversational for the transcript-based Stop hook. The public skill invokes this branch asaidlc-orchestrate.ts next team-board;isEngineToolCallstill classifies thatnextsegment as workflow engagement becauseisTerminalUtilityNext/isEngineEngagementSegmenthave noteam-boardexemption. Claude/Codex can therefore block after a read-only board query even though this marker is absent. Extend the shared engagement classifier (and add a transcript regression) for the canonicalnext team-boardform rather than relying only on the marker.
flags.orchestratorVerb !== "team-board"
harness/kiro/hooks/aidlc-kiro-adapter.ts:504
- This exemption only works when
team-boardis the first token innextArgs, but the orchestrator entrypoint accepts and strips launcher options such as--project-dirbefore parsing. Thus a validnext --project-dir <dir> team-boardreaches this guard withnextArgs[0] === "--project-dir"; after a same-turn read-only latch (for example, following--status), it is classified as bare advancing and blocked instead of being allowed through. Normalize the launcher options before applying the leading-verb exemption, using the same normalization as the engine entrypoint.
!ORCHESTRATOR_VERBS.has(nextArgs[0]) &&
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
d540946 to
4a0767b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
next team-board --config ... bypasses the shared parser and marker behavior, causing inconsistent handling of invalid arguments.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
…the freeform funnel (awslabs#1109)
4a0767b to
c0cf2c8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate unresolved issues remain involving UUID handling and Stop-hook classification.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
core/tools/aidlc-lib.ts:1969
INTENT_SELECTOR_REGEXaccepts uppercase selectors because of the/iflag, buthandleTeamBoardlater comparesintent.uuid === selectedIntent(unlike the shared resolver, which lowercases UUIDs). An uppercase form of an existing UUID therefore passes this parser and then fails to resolve; either make UUID matching case-insensitive or reject uppercase selectors instead of accepting them.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| // public grammar promises leading-token semantics. | ||
| // A leading orchestrator verb owns the command and stays on the engine path | ||
| // (see ORCHESTRATOR_VERBS); a read-only flag after it is that command's argv. | ||
| if (ORCHESTRATOR_VERBS.has(args[0])) return null; |
Summary
Routes the orchestrator's two public verbs from the
/aidlccommand surface to the engine instead of into the freeform funnel. Closes #1109.Changes
parseNextFlagsrecognises a sole leadingparkand a leadingteam-board;handleNextgains Branch 1c beside Branch 1b and emits aprintdirective namingaidlc park(run, then act on theparkeddirective) oraidlc team-board(read-only, print and stop). Both run before state inspection, like the existing terminal branches.team-boardforwards only--snapshot,--space <name>, and--intent <name>, the same allowlist discipline--doctoruses; any other token is a usage error. Selector values must match the shared name grammars (SPACE_NAME_REGEX, and a newINTENT_SELECTOR_REGEXfor record dir, slug, or uuid), since both become path segments downstream; both the engine route and the directteam-boardhandler parse the full argv through one sharedparseTeamBoardArgs, so stray tokens, duplicates, missing values, and--space ../../tmpare refused identically on both paths. Both authoredROUTESentries for the orchestrator list theteam-boardform with its selectors, soaidlc engine --helpshows it; the hiddenengine orchestratenoun previously omittedteam-boardaltogether.parkis sole-token, mirroring thehelprule, because parking mutates state.parkinside a longer sentence stays freeform.unparkreturns an error pointing at/aidlc --resume, the public spelling.ORCHESTRATOR_VERBSis exported fromaidlc-lib.tsbesideREAD_ONLY_FLAGSandWORKSPACE_VERBS.classifyTerminalCommandreturnsnullfor a leading orchestrator verb so the Kiro verb-intercept seam never runs them off-band (park mutates; team-board lives on the orchestrator, notaidlc-utility), and the Kiro tool-call guard exempts them from the bare-next backstop the way it exemptscompose. The Branch 0 latch and the--reviewincompatibility list treat the verbs as deliberate moves.team-boarddoes not touch the engine marker, so a board query stays a conversational turn for the Stop hook;parkdoes, because the park it names mutates state./aidlc park(including the Unit-scoped checkout case, where the same command parks the Unit locally and leaves shared state alone) and/aidlc team-board, and a line in the session guide. No version, badge, or CHANGELOG change, per the Release Metadata Policy.User experience
Before, over an active workflow:
After:
Checklist
Test plan
bun run check(package--check, typecheck, lint) andbun tests/gen-coverage-registry.ts --checkare green. The one lint warning in t260 reproduces on untouchedmain.parkon a fresh workspace and over an active one (print, never the new-work ask or a stage advance),parkinside a sentence stays freeform,team-boardwith and without allowlisted args plus a stray-token usage error, and soleunpark. t114 also assertsteam-boardleaves the engine marker absent whileparkwrites it. t178 pinsORCHESTRATOR_VERBSand that the classifier returnsnullfor a leading verb even with a read-only flag after it.next parkreturns the print directive, and running the named command parks the workflow and emitsparked.main(the stage question-flow precondition), unrelated to this change.Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.