Repository navigation
feat(team-setup): deterministic --directives wire + non-TTY fail-loud guard - #65
Conversation
… guard ADR-386-amendment follow-up: team setup --directives <path> validates structure (skill-owned team-helpers.sh --wire when the installed skill has it, interim 6-path JS check otherwise) and wires .adlc/init-options.json without spawning an agent session. Non-TTY runs without --directives fail loudly with the deterministic alternative instead of hanging a headless agent on the interactive skill's mode selection (~300s timeout). Workspace commands[] accepts team setup <source> --directives <path>; e2e fixture + NOTE updated.
…cepts https URLs TTY with --format json can never reach the interactive handoff (machine contract), so it refuses like non-TTY. --directives <https-url> clones (git clone into ./team-ai-directives, reusing existing clones) then wires — file:// and ssh:// rejected per the skill's Mode 1 Input Validation contract.
Fake git on PATH intercepts the https clone by copying a seeded repo (no network); asserts clone lands in ./team-ai-directives and init-options.json wires the resolved path.
realpathSync the temp parent (/var vs /private/var); node payload in git-stub.js with POSIX sh wrapper + git.cmd twin (extensionless node file hit /bin/sh on POSIX).
macOS: makeProject() canonicalizes /var -> /private/var so resolved dest compares equal. Windows: node spawnSync resolves git.exe before git.cmd, so binary interception cannot work there; the URL test takes the local-path wire branch on win32 (regex + reject paths still covered).
Loop: factory-review --self-heal (iter 0)Run |
kanfil
left a comment
kanfil
left a comment
There was a problem hiding this comment.
factory-review verdict — #65 (iter 0, self-heal)
Reviewed revision: head 298324e4a59512ed1e9c31cb09fefe171bbceb1a, base db2389610a712c979ffe67486f6e5c99e40f0287, merge-base db2389610a712c979ffe67486f6e5c99e40f0287. Exact-head detached checkout. Three-persona Round 1 (Auditor/Adversary/Pragmatist) + Round-2 cross-exam; tags via synthesize-findings.sh.
CI on this head: 10/10 SUCCESS.
🔴 Important (5) — blocks merge, fix-gated
| id | tag | finding | location |
|---|---|---|---|
| F1 | cross-validated | --format never parsed by parseArgs → refusal guard dead; on a TTY team setup --format json falls through to interactive session, contradicting the PR body's "--format fails loudly" claim |
src/dispatch.mjs:274-350, src/commands/team.mjs:200 |
| F2 | consensus | --format TTY test is vacuous — execFile children have no TTY, passes via !isTTY regardless; masks F1 |
tests/team-setup-wire.test.mjs:120-136 |
| F3→Nit | cross-validated | Credentialed --directives URL (user:TOKEN@) reaches git clone argv, .git/config, and error log; bypasses the repo's own splitGitCredentials/askpass/scrub path |
src/commands/team.mjs:143-148 |
| F5 | cross-validated | HTTPS URL silently ignored when ./team-ai-directives exists — no origin verification, re-run wires stale/wrong directives while the PR claims deterministic clone-then-wire |
src/commands/team.mjs:143-154 |
| F6 | consensus | http:// accepted despite https-only comment and error text — credentialed plaintext clone, no warning, no test coverage |
src/commands/team.mjs:143 |
(credential-exposure row above is F4 — id corrected in tags.json: findings.json F4 = credential exposure, F3 = split-write.)
💬 Nits (9, advisory — non-blocking)
- F3 (downgraded from Important by Round-2 challenge, both lanes concur):
--cwdsplit-write is unreachable — no--cwdsetter exists; latent dead code (src/commands/team.mjs:111-119) - F7: unknown valued flags silently dropped; value leaks into positional args (
src/dispatch.mjs:344) - F8:
init-options.jsonmutated before failure paths (src/commands/team.mjs:111) - F9: mock-agent guard asserts wrong file (
mock-agent.logvsADLC_MOCK_AGENT_FILE=mock.log) (tests/team-setup-wire.test.mjs:114) - Bare
--directiveswith no value silently degrades (CLI) while workspace errors loudly — inconsistent - Workspace forwarder handles only
--directives <path>, not--directives=<path> - Non-TTY guard runs after
writeInitOptions+ possible network install (fail-fast ordering) - Interim 6-path check is existence-only (file named
rulespasses) - Workspace help row omits
team setup --directivesforwarding
Verdict
🔴 not-merge-ready — 5 Important findings (spec-deviation ×2, credential exposure, silent-stale-wire, transport security). Core Phase 1 wiring works and tests pass, but the --format half of the headline claim is unimplemented (masked by a vacuous test) and the URL-handling path lacks the repo's own credential hardening.
Self-heal engaged: fix step next on the 5 synthesis-tagged actionable Important findings, then independent converge at the new head. Strictly non-approving — human code-owner approval remains mandatory.
…ne, stale-dest guard F1: parseArgs captures --format/--format= into flags.format (export parseArgs) F2: F1 regression test at parse layer; fixed mock filename; --format on URL path F4: credentialed https URL via splitGitCredentials + GIT_ASKPASS, masked logs F5: existing dest verified by remote.origin.url match, mismatch fails loudly F6: enforce ^https:// (http/file/ssh rejected) + scheme tests
Fix step (iter 0) → artifactFixed: F1 (--format parsing), F2 (genuine format-branch test + mock-log filename), F4 (credentialed-clone hygiene via workspace askpass/scrub reuse), F5 (stale-dest origin verify), F6 (https-only enforcement). Full suite 835/835 pass in fix worktree incl. F1-revert sanity. Pushed |
…y git stub)
tests 321/322 rely on a #!/bin/sh fake git on PATH; on Windows
spawnSync('git') resolves the real git.exe so the clone hits the
network (exit 128). Same precedent as provider-transport.test.mjs:1710.
https-only scheme rejection tests still run on all platforms.
Fix step (iter 1, regression on CI Windows) → artifactWin32-only: tests 321/322 used the POSIX-only git stub → real-network clone on |
kanfil
left a comment
kanfil
left a comment
There was a problem hiding this comment.
factory-review verdict — #65 (iter 1 · merge-ready)
Re-reviewed all REVIEW.md passes on new head dd4e9faabcf016334a344676d66d4a17a9188286 (fixed 298324e→dd4e9faabcf016334a344676d66d4a17a9188286). CI on this head: 10/10 pass.
Iter-0 Important set {F1, F2, F4, F5, F6} — all verified resolved at the new head by independent converge (synthesis gate re-run locally; each finding re-checked against code + functional checks, not fix-agent claims). No new Important or Nit findings introduced by the fix commits. F3 stays Nit (Round-2 downgrade stands). Residuals are documented non-blocking nits (uppercase scheme fail-loud, bare --format same-as-all-valued-flags, win32 POSIX-stub skips matching the #1710 precedent).
🟢 merge-ready — no open Important findings on dd4e9faabcf016334a344676d66d4a17a9188286. Strictly advisory: human code-owner approval and branch protection remain mandatory; this verdict approves nothing and merges nothing.
Converge (iter 1) → DONEVerified: F1, F2, F4, F5, F6 resolved; new-issue scan clean; finding set shrunk to empty (progress). No spec-correction signals. Prior findings JSON/tags re-synthesized locally. |
|
Loop complete: DONE. Verdict |
kanfil
added
the
validation
Closes the ADR-386-amendment Headless Team-Setup Constraint follow-up:
adlc-cli team setup --directives <path|https-url>wires without spawning an LLM session, and non-TTY (or--format) without--directivesfails loudly instead of hanging ~300s.Phase 1 of ADR-474 (accepted). Phase 2 flags live on adlc-team-skills/feat/team-setup-deterministic-flags; the interim JS check here is deleted in Phase 3.
Tests: 832/832 pass (incl. team-setup-wire + team-mcp-chain).